-
Notifications
You must be signed in to change notification settings - Fork 36
[WIP] Add prefect manager support #788
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
base: main
Are you sure you want to change the base?
[WIP] Add prefect manager support #788
Conversation
|
Wow! Thanks for this contribution @ptigas, this is a major addition and something I know many users will be interested in. I'm familiar with Prefect but haven't used it before so I'd appreciate feedback from others. Potentially @Andrew-S-Rosen might have comments. At first glance the code looks very clean and thank you for adding rigorous tests. This has reminded me I'll need to update the CI to drop Python 3.10 support and add 3.13. One major comment up-front - one feature of jobflow is support for dynamic flows through the If it's helpful, I'm happy to arrange a call to discuss. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this is looking to be very nicely done! I was really impressed to see how you handled the .submit() vs. serial logic. That is slick.
I would absolutely be interested in reviewing this further, but I probably won't have a chance to take a look until the middle of next week. One comment in the meantime while you continue working on the PR.
I think this PR saves me a lot of work I was planning on doing, so I may owe you... 😅
src/jobflow/managers/prefect.py
Outdated
| def run_on_prefect( | ||
| jobflow_obj: Union["jobflow.Flow", "jobflow.Job", List["jobflow.Job"]], | ||
| store: Optional["jobflow.JobStore"] = None, | ||
| task_runner: str = "concurrent", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful if this could take a str | TaskRunner as the input. For instance, if I want to use the DaskTaskRunner, which is the main way to interact with Slurm, then it would not be possible as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Andrew-S-Rosen, I've updated to take TaskRunner as an argument. I've never used DaskTaskRunner before, so I might need some help testing that part.
|
@utf @Andrew-S-Rosen thank you for the supportive comments :)
I've updated the code to support dynamic flows. I've also added an example and tested it in a prefect cluster, and it seems to be working alright. Also, I'd be happy to arrange a call to discuss the workflow I'm after in a bit more detail (interested in running atomate2 workflows in prefect, currently testing this and I'll report back or in a call about results). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file from the PR. Thanks!
| "prefect", | ||
| "pytest", | ||
| "prefect-docker>=0.6.6", | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid adding these to the core dependency stack. Having them in a dedicated prefect optional dependency section like you have done is okay.
If you want them to be tested in CI, you can include them in strict.
| tests = ["moto==5.1.10", "pytest-cov==6.2.1", "pytest==8.4.1"] | ||
| vis = ["matplotlib", "pydot"] | ||
| fireworks = ["FireWorks"] | ||
| prefect = ["prefect>=3.0.0", "prefect-docker"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the needed to have prefect-docker here? That should not be needed by all users, such as those using Prefect Cloud. I think we can just have prefect here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is better suited for documentation rather than as a module within the codebase.
|
Hi @ptigas: Thank you for your patience. Unfortunately, I remain extremely busy to review this in the depth it deserves. I will have a software engineer working with my group in September who is going to focus on Prefect + Jobflow, so we can almost certainly test things out in detail then. Until then, I have left a few minor comments in the meantime. I am happy to chat if you think it would be helpful. Just let me know. |
Summary
Include a summary of major changes in bullet points:
Additional dependencies introduced (if any)
jobflow workflows as Prefect flows. This enables users to leverage Prefect's distributed execution, monitoring, and deployment capabilities.
and examples.
Note: Updated minimum Python version requirement from 3.10 to 3.11 to align with Prefect 3.x requirements.
TODO (if any)
Checklist
Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request title.
Before a pull request can be merged, the following items must be checked:
your local machine. Start with running https://black.readthedocs.io/en/stable/index.html on your new code. This will automatically reformat
your code to PEP8 conventions and removes most issues. Then run https://pycodestyle.readthedocs.io/en/latest/, followed by
http://flake8.pycqa.org/en/latest/.
http://www.pydocstyle.org/en/2.1.1/index.html on your code.
Note that the CI system will run all the above checks. But it will be much more efficient if you already fix most errors prior to submitting
the PR. It is highly recommended that you use the pre-commit hook provided in the repository. Simply pip install pre-commit and then
pre-commit install and a check will be run prior to allowing commits.