Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

models: add Interactive Session model #90

Merged
merged 4 commits into from
Oct 9, 2020

Conversation

audrium
Copy link
Member

@audrium audrium commented Oct 5, 2020

closes #89

@audrium audrium force-pushed the interactive_session_model branch 5 times, most recently from 4fb1c3e to 31c0c65 Compare October 7, 2020 14:31
Copy link
Member

@diegodelemos diegodelemos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :) I've added a couple of comments regarding naming and relationships which shouldn't be creating much trouble in your current implementation, once we decide on them, you can create the Alembic recipe.

reana_db/models.py Outdated Show resolved Hide resolved
__tablename__ = "user_workflow_session"
__table_args__ = {"schema": "__reana"}

user_id = Column(UUIDType, ForeignKey("__reana.user_.id_"), nullable=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add the user here? I rather see this table as WorkflowSession or WorkflowInteractiveSession and the relation to User to be simply backtracked from InteractiveSession through owner_id (I see ternary relationship was mentioned in the original issue, but I have second thoughts).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good solution IMO. @tiborsimko , what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for this sprint and for MVP, it seems perfectly OK indeed.

(However, we may need to refactor this later, when we shall be addressing user group and access rights. Example: currently, a Workflow row has an owner_id, but in reality there can be several people associated with a workflow: one main owner, 2-3 collaborators having some R/W rights e.g. to rerun it, a convener having R/O rights to see the results, etc. Some collaborators could run different notebooks on the same workspace, not being owners strictly speaking... These kinds of considerations can naturally wait for later; however it is good to have a future outlook with a flexible "user-workflow-session-role" structure in mind to see where we shall be heading later.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiborsimko, should we create an issue from your comment? Or this is something to bear in mind?

reana_db/models.py Show resolved Hide resolved
jupyter = 0


class InteractiveSession(Base, Timestamp):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are missing the InteractiveSessionResource table (like WorkflowResource), we will need it to account resource usage in InteractiveSessions (more here).

For now, we should measure just CPU usage, not disk usage since now the workspace is the same as the workflow and it is already accounted (which would change if we go for https://github.com/reanahub/reana-db/pull/90/files#r501548133). Can you remember to open an issue for this, "accounting for InteractiveSessions CPU usage" once we merge this?

@mvidalgarcia
Copy link
Member

Minor: there's a typo in the last commit title.

@audrium audrium force-pushed the interactive_session_model branch 2 times, most recently from b6baafd to a560101 Compare October 9, 2020 14:41
reana_db/models.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

models: separate workflows and interactive sessions
4 participants