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.

Sign up for GitHub

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

Jump to bottom

Added reuse-executions property to pipeline schema #152

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Added reuse-executions property to pipeline schema #152

merged 2 commits into from
Jun 3, 2024

Conversation

dangquangdon
Copy link
Contributor

No description provided.

@dangquangdon dangquangdon requested a review from akx May 30, 2024 08:08
@dangquangdon dangquangdon force-pushed the dangquangdon-add-allow-reuse-property-to-pipeline-schema branch from 9cef07d to b42dff8 Compare May 30, 2024 08:12
akx
akx previously requested changes May 30, 2024
View reviewed changes
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

  1. Tests fail.
  2. The .gitignore changes seem unrelated?
  3. What are the semantics here – is this just the default for the pipeline API allow_reuse flag (which, afaiu, only affects reusing executions into execution nodes?), or something else? I can't seem to find a ref to any sort of spec for this.

@dangquangdon
Copy link
Contributor Author

3. What are the semantics here – is this just the default for the pipeline API allow_reuse flag (which, afaiu, only affects reusing executions into execution nodes?), or something else? I can't seem to find a ref to any sort of spec for this.

The idea is that if user specify this in valohai.yaml, the Automatically reuse switch button in the UI will also reflect it

@dangquangdon
Copy link
Contributor Author

  1. Tests fail.

Some kind of snapshot failed, couldn't reproduce it locally tho, I'm trying to figure it out

@hylje
Copy link
Contributor

hylje commented May 30, 2024 •
edited
Loading

The snapshots have changed due to a change in a downstream library version that changed sort order.

I've fixed the snapshot issue in #147 which is waiting for review.

@dangquangdon
Copy link
Contributor Author

The snapshots have changed due to a change in a downstream library version that changed sort order.

I've fixed the snapshot issue in #147 which is waiting for review.

Nice, thanks for the info. Did it fail also in your local machine when you were fixing it? Because it didn't happen in mine 🤔

@hylje
Copy link
Contributor

hylje commented May 30, 2024

It didn't fail at first but when I upgraded the dependencies it started failing the same way it did in CI (which always installs freshest dependencies)

@dangquangdon dangquangdon marked this pull request as draft May 30, 2024 09:50
@dangquangdon dangquangdon force-pushed the dangquangdon-add-allow-reuse-property-to-pipeline-schema branch from b42dff8 to 0c5145a Compare May 30, 2024 10:05
@dangquangdon
Copy link
Contributor Author

It didn't fail at first but when I upgraded the dependencies it started failing the same way it did in CI (which always installs freshest dependencies)

I checked the dependencies versions as well and they were the same as in CI, but after your comment, I remove the whole venv and create a fresh new one and was able to reproduce it. Seemed like it was some kind of caching issue for me.

@dangquangdon dangquangdon force-pushed the dangquangdon-add-allow-reuse-property-to-pipeline-schema branch from 0c5145a to 128fc1d Compare May 31, 2024 06:39
@dangquangdon dangquangdon marked this pull request as ready for review May 31, 2024 06:40
@dangquangdon dangquangdon dismissed akx’s stale review May 31, 2024 06:41

renamed to reuse-executions and fixed tests

@dangquangdon dangquangdon changed the title Added allow-reuse property to pipeline schema Added reuse-executions property to pipeline schema May 31, 2024
@dangquangdon dangquangdon requested a review from akx June 3, 2024 05:45
akx
akx reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

LGTM (not approve-gating yet), if @tarekoraby is OK with the new name.

@tarekoraby
Copy link

reuse-executions LGTM 👍

@akx akx merged commit 03bf09c into master Jun 3, 2024
6 checks passed
@akx akx deleted the dangquangdon-add-allow-reuse-property-to-pipeline-schema branch June 3, 2024 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarekoraby tarekoraby tarekoraby left review comments

@akx akx akx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dangquangdon @hylje @tarekoraby @akx