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

Introduce Snapshot entity in models #1240

Merged
merged 15 commits into from
Sep 2, 2022

Conversation

fruttasecca
Copy link
Member

@fruttasecca fruttasecca commented Aug 29, 2022

Description

This PR introduces Snapshots as an orchest-api models entity. I've tried to minimize the exposure/knowledge of the client and the orchest-webserver to this new entity given that the intent was to support another feature (switching between pipelines for a DRAFT job) without closing doors for the future.

As it is, a job depends on a snapshot through a foreign key column job.snapshoot_uuid, existing jobs are migrated in this migration 44699af. The reason is that inverting the dependency felt like a stronger decision and that currently, a snapshot is created before the job is, making the job effectively depend on the snapshot.

Checklist

  • I have manually tested my changes and I am happy with the result.
  • I haven't introduced breaking changes that would disrupt existing jobs, i.e. backwards compatibility is maintained.
  • In case I changed one of the services' models.py I have performed the appropriate database migrations (refer to the DB migration docs).

Open questions:

  • the snapshot db record doesn't store anything related to the actual location of the snapshot, the application still uses the implicit path given by the project_uuid and pipeline_uuid. Do we want to change this? Likely not needed until/if we expand the scope of snapshots
  • given the information within a snapshot like the pipeline_definition and the pipeline path we could, theoretically, slim down the job model, e.g. by dropping the pipeline definition, the run spec, etc. I'm not sure it's worth doing that while tests are still disabled since an error here could disrupt existing jobs

@fruttasecca fruttasecca changed the title Introduce snapshot entity in models Introduce Snapshot entity in models Aug 29, 2022
@fruttasecca fruttasecca added the internal Something that does not affect users directly, e.g. code quality. label Aug 29, 2022
@fruttasecca fruttasecca changed the base branch from dev to feat/ui-overhaul-jobs August 30, 2022 11:14
@yannickperrenet
Copy link
Contributor

Let me first reply to the open questions from the PR description:

the snapshot db record doesn't store anything related to the actual location of the snapshot, the application still uses the implicit path given by the project_uuid and pipeline_uuid. Do we want to change this? Likely not needed until/if we expand the scope of snapshots

Actually... 🤔 I think that adding the location of the snapshot as a db record might make it easier to later make "snapshots" a broader concept in Orchest. For example, let's say we want to have userdir/snapshots (just like we have userdir/jobs) then getting to that world is easier if we store the location (I think). We can always add the location as a DB record to snapshots later if anything changes in the future 👍

given the information within a snapshot like the pipeline_definition and the pipeline path we could, theoretically, slim down the job model, e.g. by dropping the pipeline definition, the run spec, etc. I'm not sure it's worth doing that while tests are still disabled since an error here could disrupt existing jobs

Quite a nice idea! And agree with the caution of not doing it now. Could you open an issue for this (good idea to keep track of this technical debt).

fruttasecca and others added 3 commits September 1, 2022 09:13
Co-authored-by: Yannick Perrenet <26223174+yannickperrenet@users.noreply.github.com>
…eline-of-a-job-draft

Dynamically update the pipeline of a job draft
@fruttasecca
Copy link
Member Author

We can always add the location as a DB record to snapshots later if anything changes in the future +1

Indeed

@fruttasecca fruttasecca merged commit 253e759 into feat/ui-overhaul-jobs Sep 2, 2022
@fruttasecca fruttasecca deleted the feat/backend-project-snapshots branch September 2, 2022 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Something that does not affect users directly, e.g. code quality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants