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

Update dependencies for python=312 #846

Merged
merged 11 commits into from
Oct 7, 2024
Merged

Conversation

lorenzocerrone
Copy link
Collaborator

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

Copy link

github-actions bot commented Oct 7, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  fractal_tasks_core
  masked_loading.py
  fractal_tasks_core/cellvoyager
  metadata.py
  fractal_tasks_core/roi
  v1_overlaps.py
  fractal_tasks_core/tasks
  cellvoyager_to_ome_zarr_init.py
  cellvoyager_to_ome_zarr_init_multiplex.py
Project Total  

This report was generated by python-coverage-comment-action

@tcompa
Copy link
Collaborator

tcompa commented Oct 7, 2024

@lorenzocerrone with poetry you should remember to update the actual dependency version, e.g. with poetry lock. I did it in 3581528, where you see e.g. that pandas went from 1.5.3 to 2.2.3.

@tcompa
Copy link
Collaborator

tcompa commented Oct 7, 2024

I see several FutureWarnings in the CI, and at least some of them seem pandas-related - see example below. I also see that the pandas version is now unbound.

I suggest we take one of these options:

  1. We cap pandas to 2.2.*, and ignore the warnings for the time being - hoping that many of them will be gone when switching to ngio.
  2. If we really want to avoid capping it, we should address the warnings (starting with pytest -W error::FutureWarning, and then finding the precise causes to be fixed).

  /home/runner/work/fractal-tasks-core/fractal-tasks-core/fractal_tasks_core/cellvoyager/metadata.py:211: FutureWarning: Calling int on a single element Series is deprecated and will raise a TypeError in the future. Use int(ser.iloc[0]) instead
    column_count = int(meas_df["ColumnCount"])

tests/test_unit_parse_yokogawa_metadata.py: 15 warnings
  /opt/hostedtoolcache/Python/3.9.20/x64/lib/python3.9/site-packages/numpy/core/fromnumeric.py:86: FutureWarning: The behavior of DataFrame.sum with axis=None is deprecated, in a future version this will reduce over both axes and return a scalar. To retain the old behavior, pass axis=0 (or do not pass axis)
    return reduction(axis=axis, out=out, **passkwargs)

tests/test_unit_parse_yokogawa_metadata.py: 1141 warnings
  /home/runner/work/fractal-tasks-core/fractal-tasks-core/fractal_tasks_core/roi/_overlaps_common.py:57: FutureWarning: Series.__getitem__ treating keys as positions is deprecated. In a future version, integer keys will always be treated as labels (consistent with DataFrame behavior). To access a value by position, use `ser.iloc[pos]`
    [box1[0], box1[2]], [box2[0], box2[2]], tol=tol

tests/test_unit_parse_yokogawa_metadata.py: 1141 warnings
  /home/runner/work/fractal-tasks-core/fractal-tasks-core/fractal_tasks_core/roi/_overlaps_common.py:60: FutureWarning: Series.__getitem__ treating keys as positions is deprecated. In a future version, integer keys will always be treated as labels (consistent with DataFrame behavior). To access a value by position, use `ser.iloc[pos]`
    [box1[1], box1[3]], [box2[1], box2[3]], tol=tol

@tcompa
Copy link
Collaborator

tcompa commented Oct 7, 2024

Last comment: would it make sense to also address #821 here?

@jluethi
Copy link
Collaborator

jluethi commented Oct 7, 2024

Looks generally good to me. Unlocking torch can lead to issues in some deployments, but we anyway pin torch on UZH installations through "Add pinned packages" already, so we have a mechanism for that.
+1 in checking on whether we can also unpin numpy.

Whether we should pin pandas or address the issues: Some may go away with ngio. The cellvoyager/metadata.py related ones would probably stay. So we should at least address those here eventually

@lorenzocerrone
Copy link
Collaborator Author

lorenzocerrone commented Oct 7, 2024

I see several FutureWarnings in the CI, and at least some of them seem pandas-related - see example below. I also see that the pandas version is now unbound.

I suggest we take one of these options:

  1. We cap pandas to 2.2.*, and ignore the warnings for the time being - hoping that many of them will be gone when switching to ngio.
  2. If we really want to avoid capping it, we should address the warnings (starting with pytest -W error::FutureWarning, and then finding the precise causes to be fixed).
  • After looking at the warnings, I went for option 2. There were only a handful of direct __getitem___ calls causing the warnings.

  • About numpy, I have run some local tests with different versions of numpy without issues.
    In the end I followed Relax numpy dependency to <=2.1.0 #821 and fix numpy<=2.1.0

@lorenzocerrone with poetry you should remember to update the actual dependency version, e.g. with poetry lock. I did it in 3581528, where you see e.g. that pandas went from 1.5.3 to 2.2.3.

  • Sorry, my bad, but I ran the poetry lock --no-update (without removing the pre-existing lock file). Is that not enough for poetry to update all dependencies?

@tcompa
Copy link
Collaborator

tcompa commented Oct 7, 2024

Sorry, my bad, but I ran the poetry lock --no-update (without removing the pre-existing lock file). Is that not enough for poetry to update all dependencies?

No, this is not enough:

By default, this will lock all dependencies to the latest available compatible versions. To only refresh the lock file, use the --no-update option.
--no-update: Do not update locked versions, only refresh lock file.
(https://python-poetry.org/docs/cli/#lock)

@tcompa
Copy link
Collaborator

tcompa commented Oct 7, 2024

About numpy, I have run some local tests with different versions of numpy without issues.

I don't know precisely the reason, but poetry is not automatically updating numpy to v2 in the lock file. This is most likely due to some funny pin on astropy (astropy/astropy#15234), where they stopped supporting numpy v2 after some version.

If you ran tests locally, that's good. Moreover, the pip-based tests (that will fully run after you merge to main) will install whatever latest compatible version they find.

@lorenzocerrone lorenzocerrone merged commit c9cb1d5 into main Oct 7, 2024
23 checks passed
@lorenzocerrone lorenzocerrone deleted the update_dependencies_py312 branch October 7, 2024 18:51
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.

3 participants