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

Jumpster #136

Merged
merged 13 commits into from
Apr 25, 2024
Merged

Jumpster #136

merged 13 commits into from
Apr 25, 2024

Conversation

manuelma
Copy link
Collaborator

@manuelma manuelma commented Apr 20, 2024

This PR removes the dagster dependency. It implements a new small module that does the dagster thing for us. I called it jumpster because it also supports jumps (i.e., loops).

I have to say this was very straightforward to do, mainly code reorganization. The only thing I had to reimplement from scratch was the get_steps_to_execute function from the ActiveExecution class, but it was relatively simple. This is to support the point that the bit of dagster that we use is really minimal.

All tests in all repositories pass but we'd probably need to test it with real workflows before merging - if we ever want to merge it. We'd also need to remove the dasgter dep from setuptools etc - I haven't done that yet - forgot how to do it - but maybe someone can help.

Fixes spine-tools/Spine-Toolbox#2523

Checklist before merging

  • Documentation (also in Toolbox repo) is up-to-date
  • Release notes in Toolbox repo have been updated
  • Unit tests have been added/updated accordingly
  • Code has been formatted by black
  • Unit tests pass

@manuelma manuelma marked this pull request as draft April 20, 2024 08:25
@manuelma manuelma changed the base branch from master to 0.8-dev April 20, 2024 08:26
@manuelma manuelma marked this pull request as ready for review April 20, 2024 08:26
@ptsavol
Copy link
Member

ptsavol commented Apr 22, 2024

Sounds good! Can you merge the latest commit from 0.8-dev to jumpster branch, please.

We'd also need to remove the dasgter dep from setuptools etc - I haven't done that yet - forgot how to do it - but maybe someone can help.

Maybe just remove "dagster>=0.12.6, <0.12.9", line from pyproject.toml. Is there something else to it? There are a bunch of version limits (eg. pendulum < 3.0) all over the place that are there only because we require dagster 0.12.8, but we can worry about those later.

@manuelma
Copy link
Collaborator Author

I was hoping somebody else could pick this up and finish it off? I have very limited time this week. What you say @PekkaSavolainen sounds right, maybe you can go ahead and do it? Sorry about that.

Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 85.35032% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 62.86%. Comparing base (ba6be8a) to head (e54479f).

Files Patch % Lines
spine_engine/jumpster.py 84.83% 31 Missing and 11 partials ⚠️
spine_engine/spine_engine.py 88.00% 1 Missing and 2 partials ⚠️
spine_engine/utils/helpers.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           0.8-dev     #136      +/-   ##
===========================================
+ Coverage    62.53%   62.86%   +0.33%     
===========================================
  Files           46       43       -3     
  Lines         3918     3886      -32     
  Branches       763      765       +2     
===========================================
- Hits          2450     2443       -7     
+ Misses        1312     1296      -16     
+ Partials       156      147       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ptsavol
Copy link
Member

ptsavol commented Apr 22, 2024

Jumpster is quite good but I found a problem. After executing a Jump Link, all the items in the loop are executed in parallel. You can see the problem by executing the loop_condition_with_cmd_line_args execution test project, which fails because of this problem.

@manuelma
Copy link
Collaborator Author

manuelma commented Apr 23, 2024

Jumpster is quite good but I found a problem. After executing a Jump Link, all the items in the loop are executed in parallel. You can see the problem by executing the loop_condition_with_cmd_line_args execution test project, which fails because of this problem.

I can fix.

Edit: should be better now.

manuelma and others added 4 commits April 23, 2024 09:22
- Don't discard step from iterating active unless they finished,
  otherwise they might execute out of order.
- Make sure steps run with the most recent output.
Execution direction was accidentally converted to string in
SpineEngine._stop_item()

Re spine-tools/Spine-Toolbox#2523
@ptsavol ptsavol merged commit 243280a into 0.8-dev Apr 25, 2024
10 checks passed
@ptsavol ptsavol deleted the jumpster branch April 25, 2024 08:56
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.

Ditch Dagster
3 participants