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

Python upgrade 39 discussion #427

Merged
merged 28 commits into from
Jun 23, 2024
Merged

Python upgrade 39 discussion #427

merged 28 commits into from
Jun 23, 2024

Conversation

zain-sohail
Copy link
Member

@zain-sohail zain-sohail commented Jun 20, 2024 •
edited by rettigl
Loading

There are plenty of issues that come out when upgrading version since it also allows the other libraries to be updated.
Here we can discuss how to handle them.

Closes #368

@rettigl
Copy link
Member

rettigl commented Jun 21, 2024

Most of the issues seem to be with astropy, which is incompatible with the installed numpy version. Most recent astropy versions only allow python>=3.10, so maybe we need to fix this by limiting numpy manually.

@coveralls
Copy link
Collaborator

coveralls commented Jun 21, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9617177796

Details

  • 39 of 40 (97.5%) changed or added relevant lines in 3 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 91.883%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/calibrator/momentum.py 10 11 90.91%
Files with Coverage Reduction New Missed Lines %
sed/calibrator/momentum.py 6 86.78%
Totals Coverage Status
Change from base Build 9572471667: -0.08%
Covered Lines: 6452
Relevant Lines: 7022
💛 - Coveralls

@coveralls
Copy link
Collaborator

coveralls commented Jun 21, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9617176957

Details

  • 39 of 40 (97.5%) changed or added relevant lines in 3 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 91.883%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/calibrator/momentum.py 10 11 90.91%
Files with Coverage Reduction New Missed Lines %
sed/calibrator/momentum.py 6 86.78%
Totals Coverage Status
Change from base Build 9572471667: -0.08%
Covered Lines: 6452
Relevant Lines: 7022
💛 - Coveralls

@coveralls
Copy link
Collaborator

coveralls commented Jun 21, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9618859347

Details

  • 36 of 37 (97.3%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 91.879%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/calibrator/momentum.py 10 11 90.91%
Files with Coverage Reduction New Missed Lines %
sed/calibrator/momentum.py 6 86.78%
Totals Coverage Status
Change from base Build 9572471667: -0.08%
Covered Lines: 6449
Relevant Lines: 7019
💛 - Coveralls

@rettigl
Copy link
Member

rettigl commented Jun 21, 2024

To me, currently the only issue seems to be dask for versions > 2023.12.0. The introduced a new method for computing there, "Query Planning", which breaks for some of our functions. See dask/dask#10995 for a discussion, and the changelog to 2023.3.0.
As an intermediate solution, I have limited dask to 2023.12.0, which does not make problems.

@rettigl
Copy link
Member

rettigl commented Jun 21, 2024

I'll make a separate PR into this one to update type annotations to python 3.9

@rettigl rettigl changed the base branch from main to v1_feature_branch June 21, 2024 21:39
@rettigl
Copy link
Member

rettigl commented Jun 21, 2024

I changed the target branch to v1_feature_branch, because I would suggest to only bring in these changes together with all the other breaking changes we are collecting for v1

@coveralls
Copy link
Collaborator

coveralls commented Jun 22, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9626531063

Details

  • 36 of 37 (97.3%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 91.879%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/calibrator/momentum.py 10 11 90.91%
Files with Coverage Reduction New Missed Lines %
sed/calibrator/momentum.py 6 86.78%
Totals Coverage Status
Change from base Build 9588067031: -0.04%
Covered Lines: 6449
Relevant Lines: 7019
💛 - Coveralls

@zain-sohail
Copy link
Member Author

To me, currently the only issue seems to be dask for versions > 2023.12.0. The introduced a new method for computing there, "Query Planning", which breaks for some of our functions. See dask/dask#10995 for a discussion, and the changelog to 2023.3.0. As an intermediate solution, I have limited dask to 2023.12.0, which does not make problems.

Dask issues still present in higher python versions. Here's tests with 3.12. Not sure if it's the same issue: https://github.com/OpenCOMPES/sed/actions/runs/9626557198

@zain-sohail zain-sohail mentioned this pull request Jun 22, 2024
Merged
@rettigl
Copy link
Member

rettigl commented Jun 22, 2024

Dask issues still present in higher python versions. Here's tests with 3.12. Not sure if it's the same issue: https://github.com/OpenCOMPES/sed/actions/runs/9626557198

That's this here: dask/dask#11035

Not sure how to fix, though, because going to a more recent dask version does not work at the moment...

@coveralls
Copy link
Collaborator

coveralls commented Jun 22, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9627802081

Details

  • 36 of 37 (97.3%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 91.879%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/calibrator/momentum.py 10 11 90.91%
Files with Coverage Reduction New Missed Lines %
sed/calibrator/momentum.py 6 86.78%
Totals Coverage Status
Change from base Build 9588067031: -0.04%
Covered Lines: 6449
Relevant Lines: 7019
💛 - Coveralls

@rettigl rettigl force-pushed the python-upgrade-39 branch from f2cdd0f to 030395c Compare June 22, 2024 19:47
@rettigl
Copy link
Member

rettigl commented Jun 22, 2024

Limiting the package to <3.12.3 and excludint 3.11.9 provides a temporary fix, but on the long run we need to look into this issue.

@coveralls
Copy link
Collaborator

coveralls commented Jun 22, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9627894556

Details

  • 36 of 37 (97.3%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 91.879%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/calibrator/momentum.py 10 11 90.91%
Files with Coverage Reduction New Missed Lines %
sed/calibrator/momentum.py 6 86.78%
Totals Coverage Status
Change from base Build 9588067031: -0.04%
Covered Lines: 6449
Relevant Lines: 7019
💛 - Coveralls

@zain-sohail
Copy link
Member Author

Limiting the package to <3.12.3 and excludint 3.11.9 provides a temporary fix, but on the long run we need to look into this issue.

Thanks for the fix. Let's merge this for now and we can make a new issue/PR for fixing dask problems.

@coveralls
Copy link
Collaborator

coveralls commented Jun 22, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9628968309

Details

  • 180 of 181 (99.45%) changed or added relevant lines in 41 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 91.886%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sed/calibrator/momentum.py 11 12 91.67%
Files with Coverage Reduction New Missed Lines %
sed/core/metadata.py 1 93.22%
sed/calibrator/momentum.py 4 87.03%
Totals Coverage Status
Change from base Build 9588067031: -0.03%
Covered Lines: 6432
Relevant Lines: 7000
💛 - Coveralls

@zain-sohail zain-sohail merged commit e4c1f5f into v1_feature_branch Jun 23, 2024
5 checks passed
@zain-sohail zain-sohail deleted the python-upgrade-39 branch June 23, 2024 12:34
@zain-sohail zain-sohail mentioned this pull request Jun 23, 2024
Closed
@zain-sohail
Copy link
Member Author

Weird that it failed. Maybe it was the type annotations.
https://github.com/OpenCOMPES/sed/actions/runs/9633480796

@rettigl
Copy link
Member

rettigl commented Jun 23, 2024

Ah, that's poetry sometimes removing setup tools. We should add it to the requirements as quick fix.

rettigl
rettigl reviewed Jun 23, 2024
View reviewed changes
Copy link
Member

@rettigl rettigl left a comment

Choose a reason for hiding this comment

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

Some fixes to add.

sed/calibrator/momentum.py Show resolved Hide resolved
sed/calibrator/energy.py Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
sed/calibrator/momentum.py Show resolved Hide resolved
sed/calibrator/momentum.py Show resolved Hide resolved
sed/core/processor.py Show resolved Hide resolved
sed/core/processor.py Show resolved Hide resolved
sed/loader/flash/loader.py Show resolved Hide resolved
sed/loader/sxp/loader.py Show resolved Hide resolved
sed/loader/sxp/loader.py Show resolved Hide resolved
@rettigl rettigl restored the python-upgrade-39 branch June 23, 2024 18:23
@rettigl rettigl mentioned this pull request Jun 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rettigl rettigl rettigl left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@zain-sohail @rettigl @coveralls