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

feat(snakemake): upgrade to Snakemake 8.27.1 #476

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

Alputer
Copy link
Member

@Alputer Alputer commented Nov 5, 2024

Remove unnecessary snakemake dependencies. They are already included in the snakemake package.

@Alputer Alputer requested a review from tiborsimko November 5, 2024 14:32
@Alputer Alputer self-assigned this Nov 5, 2024
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 41.55%. Comparing base (8f0bde7) to head (4d01234).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
reana_commons/version.py 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #476   +/-   ##
=======================================
  Coverage   41.55%   41.55%           
=======================================
  Files          28       28           
  Lines        2024     2024           
=======================================
  Hits          841      841           
  Misses       1183     1183           
Files with missing lines Coverage Δ
reana_commons/version.py 0.00% <0.00%> (ø)

@@ -41,10 +41,6 @@
"snakemake==7.32.4 ; python_version<'3.11'",
"pulp>=2.7.0,<2.8.0 ; python_version<'3.11'",
"snakemake==8.24.1 ; python_version>='3.11'",
"snakemake-interface-common==1.17.4 ; python_version>='3.11'",
Copy link
Member

Choose a reason for hiding this comment

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

todo: ‏Please explain in the commit message body why this is OK, for example that the upstream Snakemake 8 version already provides these packages as default. (After you check with py311 and above.)

Copy link
Member

Choose a reason for hiding this comment

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

todo: ‏For the release commit, please use capital S in Snakemake.

Copy link
Member

Choose a reason for hiding this comment

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

todo: ‏Please rebase against latest master.

Copy link
Member

Choose a reason for hiding this comment

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

question: ‏BTW if you are in the middle of retesting various Snakemake examples and Python versions, perhaps you could have a look whether we cannot upgrade from 8.24.1 to a higher officially released Snakemake version? (Currently 8.27.1.) This is fully optional, as we can also do it later.

Copy link
Member

Choose a reason for hiding this comment

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

After you check with py311 and above.

BTW I checked locally with tox and everything is OK except for Python 3.13:

$ tox
  py38: OK (41.41=setup[26.56]+cmd[14.85] seconds)
  py39: OK (48.24=setup[33.53]+cmd[14.71] seconds)
  py310: OK (46.67=setup[31.93]+cmd[14.74] seconds)
  py311: OK (16.04=setup[1.22]+cmd[14.82] seconds)
  py312: OK (43.06=setup[25.77]+cmd[17.29] seconds)
  py313: FAIL code 1 (247.95 seconds)

The problem with Python 3.13 are related to building the datrie extension:

...
      building 'datrie' extension
      creating build/temp.linux-x86_64-cpython-313/src
      gcc -fno-strict-overflow -Wsign-compare -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -fexceptions -fcf-protection -fexceptions -fcf-protection -fexceptions -fcf-protection -O3 -fPIC -Ilibdatrie -I/home/tibor/private/project/reana/src/reana-commons/.tox/py313/include -I/usr/include/python3.13 -c src/datrie.c -o build/temp.linux-x86_64-cpython-313/src/datrie.o
      src/datrie.c: In function ‘__pyx_pf_6datrie_8BaseTrie___init__’:
      src/datrie.c:5669:53: error: passing argument 1 of ‘trie_new’ from incompatible pointer type [-Wincompatible-pointer-types]
       5669 |   __pyx_v_self->_c_trie = trie_new(__pyx_v_alpha_map->_c_alpha_map);
            |                                    ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
            |                                                     |
            |                                                     struct AlphaMap *
      In file included from src/datrie.c:1248:

This is most likely due to datrie troubles with gcc 14.2.1 which is the version that I'm using.

A workaround is to set flags:

$ export CFLAGS="-Wno-error=incompatible-pointer-types" ; export CXXFLAGS="-Wno-error=incompatible-pointer-types"; tox -e py313
  py313: OK (47.39=setup[31.21]+cmd[16.18] seconds)
  congratulations :) (47.62 seconds)

See also pytries/datrie#101

Alputer added a commit to Alputer/reana-commons that referenced this pull request Jan 27, 2025
@Alputer Alputer force-pushed the snakemake-dependency-remove branch from c75ed7d to 5dfa1c4 Compare January 27, 2025 09:47
Alputer added a commit to Alputer/reana-commons that referenced this pull request Jan 27, 2025
We remove the unnecessary dependencies that are already included in the snakemake python package and update the snakemake version. It is better to rely on snakemake to handle these dependencies instead of setting them manually in REANA.
@Alputer Alputer force-pushed the snakemake-dependency-remove branch from 5dfa1c4 to f56e7c9 Compare January 27, 2025 11:55
@Alputer Alputer changed the title build(python): remove unnecessary snakemake dependencies (#476) build(python): remove unnecessary Snakemake dependencies Jan 27, 2025
Alputer added a commit to Alputer/reana-commons that referenced this pull request Jan 27, 2025
Remove redundant dependencies already included in the Snakemake package and update the Snakemake version. Relying on Snakemake to manage these dependencies is more efficient and less error-prone than defining them manually in REANA.
@Alputer Alputer force-pushed the snakemake-dependency-remove branch from f56e7c9 to e9de5ff Compare January 27, 2025 11:57
Alputer added a commit to Alputer/reana-commons that referenced this pull request Jan 27, 2025
Remove redundant dependencies already included in the Snakemake
 package and update the Snakemake version. Relying on Snakemake
 to manage these dependencies is more efficient and less error-prone
 than defining them manually in REANA.
@Alputer Alputer force-pushed the snakemake-dependency-remove branch from e9de5ff to 897197b Compare January 27, 2025 11:58
Remove redundant dependencies already included in the Snakemake
package and update the Snakemake version. Relying on Snakemake
to manage these dependencies is more efficient and less error-prone
than defining them manually in REANA.
@Alputer Alputer force-pushed the snakemake-dependency-remove branch from 8b757a1 to 4d01234 Compare January 27, 2025 14:34
@tiborsimko tiborsimko changed the title build(python): remove unnecessary Snakemake dependencies feat(snakemake): upgrade to Snakemake 8.27.1 Jan 27, 2025
@tiborsimko tiborsimko merged commit 4d01234 into reanahub:master Jan 27, 2025
16 checks passed
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.

2 participants