-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix(snakemake): update to v7.32.4 to make Python 3.11 clients compatible #435
Conversation
673d95f
to
c916eb3
Compare
…anahub#435) Update Snakemake version to v7.32.4 (latest one before v8 refactoring), to support newer features and resolve problems for clients using Python 3.11. Other than updating the dependency and Snakemake base image, change `reana_commons/snakemake.py` to switch from the `first_rule` Snakemake workflow directive to the `default_target` one, to adhere to the changes made in snakemake/snakemake!638ec1a. Closes: reanahub/reana-client#655. Closes: reanahub/reana-workflow-engine-snakemake#31.
c916eb3
to
f711b52
Compare
…anahub#435) Update Snakemake version to v7.32.4 (latest one before v8 refactoring), to support newer features and resolve problems for clients using Python 3.11. Other than updating the dependency and Snakemake base image, change `reana_commons/snakemake.py` to switch from the `first_rule` Snakemake workflow directive to the `default_target` one, to adhere to the changes made in snakemake/snakemake!638ec1a. Closes: reanahub/reana-client#655. Closes: reanahub/reana-workflow-engine-snakemake#31.
f711b52
to
4830c7b
Compare
…anahub#435) Update Snakemake version to v7.32.4 (latest one before v8 refactoring), to support newer features and resolve problems for clients using Python 3.11. Other than updating the dependency and Snakemake base image, change `reana_commons/snakemake.py` to switch from the `first_rule` Snakemake workflow directive to the `default_target` one, to adhere to the changes made in snakemake/snakemake!638ec1a. Closes: reanahub/reana-client#655. Closes: reanahub/reana-workflow-engine-snakemake#31.
4830c7b
to
10df05a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #435 +/- ##
==========================================
+ Coverage 36.36% 36.38% +0.01%
==========================================
Files 26 26
Lines 1573 1575 +2
==========================================
+ Hits 572 573 +1
- Misses 1001 1002 +1
|
…anahub#435) Update Snakemake version to v7.32.4 (latest one before v8 refactoring), to support newer features and resolve problems for clients using Python 3.11. Other than updating the dependency and Snakemake base image, change `reana_commons/snakemake.py` to switch from the `first_rule` Snakemake workflow directive to the `default_target` one, to adhere to the changes made in snakemake/snakemake!638ec1a. Closes: reanahub/reana-client#655. Closes: reanahub/reana-workflow-engine-snakemake#31.
10df05a
to
99871ee
Compare
if hasattr(workflow, "_persistence"): | ||
workflow._persistence = Persistence(dag=dag) | ||
else: | ||
# for backwards compatibility (Snakemake < 7 for Python 3.6) | ||
workflow.persistence = Persistence(dag=dag) |
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.
This is needed to also support Python 3.6, that uses a Snakemake version (6.15) in which the new default_target
directive is present, but internally persistence
had not been replaced by _persistence
yet in the snakemake.Workflow
class. I think this approach of directly checking the attribute (rather than checking, say, the Python version) is correct, but I'm more than happy to get advice on how to do this better, if you think there's a more appropriate way!
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 good and I prefer it over checking Snakemake/Python version
@@ -440,7 +440,7 @@ def default_workspace(): | |||
REANA_WORKFLOW_ENGINES = ["yadage", "cwl", "serial", "snakemake"] | |||
"""Available workflow engines.""" | |||
|
|||
REANA_DEFAULT_SNAKEMAKE_ENV_IMAGE = "docker.io/snakemake/snakemake:v6.8.0" |
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.
This change should rather be a feature, not a fix. While it's technically true that we are fixing things for py311 clients, we are also upgrading Snakemake to a new major version, which might affect all clients for all other Python versions. So we should rather bring attention to that. I would suggest putting something like feat(snakemake): upgrade to Snakemake 7.32.4
for the release announcement headline.
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.
Working well! Also reana-client works with Python 3.11+.
I went through the code and everything seems good, except for one thing that we should modify in r-w-e-snakemake. In particular, the method _wait_for_jobs
is now an async coroutine (see commit snakemake/snakemake@50b8f16). I think the code still works because the coroutine is called like this (source):
def _wait_thread(self):
try:
asyncio.run(self._wait_for_jobs())
except Exception as e:
print(e)
self.workflow.scheduler.executor_error_callback(e)
So when _wait_for_jobs
is called it should return immediately the coroutine, but in our case it is just a function, so it is executed as normal. Given the while True
, the function never returns until self.wait
is set to false; at that point, the non-async _wait_for_jobs
returns None
, and asyncio.run()
fails after all the jobs are already executed. Indeed, at the end of the logs I see:
a coroutine was expected, got None
PS: there is also the small issue with the missing user/home directory that we have talked about
if hasattr(workflow, "_persistence"): | ||
workflow._persistence = Persistence(dag=dag) | ||
else: | ||
# for backwards compatibility (Snakemake < 7 for Python 3.6) | ||
workflow.persistence = Persistence(dag=dag) |
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 good and I prefer it over checking Snakemake/Python version
if sys.version_info.major == 3 and sys.version_info.minor in (11, 12): | ||
pytest.xfail( | ||
"Snakemake features of reana-client are not supported on Python 3.11" | ||
) |
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.
This should also be removed from tests in reana-client
Update Snakemake version to v7.32.4 (latest one before v8 refactoring), to support newer features and resolve problems for clients using Python 3.11. Other than updating the dependency and Snakemake base image, change `reana_commons/snakemake.py` to switch from the `first_rule` Snakemake workflow directive to the `default_target` one, to adhere to the changes made in snakemake/snakemake!638ec1a. Closes: reanahub/reana-client#655 Closes: reanahub/reana-workflow-engine-snakemake#31
99871ee
to
b8caf55
Compare
…#699) After upgrading Snakemake to version 7.32.4 (reanahub/reana-commons#435) there is no need to avoid running Snakemake tests on Python 3.11 and 3.12, as it should be supported. Closes reanahub#655
…#700) After upgrading Snakemake to version 7.32.4 (reanahub/reana-commons#435) there is no need to avoid running Snakemake tests on Python 3.11 and 3.12, as it should be supported. Closes reanahub#655
Update Snakemake version to v7.32.4 (latest one before v8 refactoring), to support newer features and resolve problems for clients using Python 3.11. Other than updating the dependency and Snakemake base image, change `reana_commons/snakemake.py` to switch from the `first_rule` Snakemake workflow directive to the `default_target` one, to adhere to the changes made in snakemake/snakemake!638ec1a. Closes: reanahub/reana-client#655 Closes: reanahub/reana-workflow-engine-snakemake#31
b8caf55
to
40ff804
Compare
…#700) After upgrading Snakemake to version 7.32.4 (reanahub/reana-commons#435) there is no need to avoid running Snakemake tests on Python 3.11 and 3.12, as it should be supported. Closes reanahub#655
Update Snakemake version to v7.32.4 (latest one before v8 refactoring), to support newer features and resolve problems for clients using Python 3.11. Other than updating the dependency and Snakemake base image, change `reana_commons/snakemake.py` to switch from the `first_rule` Snakemake workflow directive to the `default_target` one, to adhere to the changes made in snakemake/snakemake!638ec1a. Closes: reanahub/reana-client#655 Closes: reanahub/reana-workflow-engine-snakemake#31
40ff804
to
20ae9ce
Compare
…#700) After upgrading Snakemake to version 7.32.4 (reanahub/reana-commons#435) there is no need to avoid running Snakemake tests on Python 3.11 and 3.12, as it should be supported. Closes reanahub#655
…#700) After upgrading Snakemake to version 7.32.4 (reanahub/reana-commons#435) there is no need to avoid running Snakemake tests on Python 3.11 and 3.12, as it should be supported. Closes reanahub#655
…nahub#700) After upgrading Snakemake to version 7.32.4 (reanahub/reana-commons#435) there is no need to avoid running Snakemake tests on Python 3.11 and 3.12, as it should be supported. Closes reanahub#655
Update Snakemake version to v7.32.4 (latest one before v8 refactoring),
to support newer features and resolve problems for clients using Python 3.11.
reana_commons/config.py
: update default Snakemake environment image.reana_commons/snakemake.py
: modifyfirst_rule
directive todefault_target
directive to adhere to the changes made insnakemake/snakemake!638ec1a.
setup.py
: adjust Snakemake version in the dependencies.Closes: reanahub/reana-client#655.
Closes: reanahub/reana-workflow-engine-snakemake#31.
Note that, even though the external Snakemake API did not change, quite a few things
are different internally from v6.8.0 to v7.32.4, so proper testing is needed.
Also note that bigger efforts in improving the way in which Snakemake and REANA
interact are probably better suited when upgrading to Snakemake v8 and use a plugin-based
approach.