fractal-analytics-platform / fractal-tasks-core

Main tasks for the Fractal analytics platform
https://fractal-analytics-platform.github.io/fractal-tasks-core/
BSD 3-Clause "New" or "Revised" License
14 stars 6 forks source link

Update dependencies for python=312 #846

Closed lorenzocerrone closed 1 month ago

lorenzocerrone commented 1 month ago

Checklist before merging

github-actions[bot] commented 1 month ago

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 commented 1 month ago

@lorenzocerrone with poetry you should remember to update the actual dependency version, e.g. with poetry lock. I did it in https://github.com/fractal-analytics-platform/fractal-tasks-core/pull/846/commits/3581528ae698fabb5ebe5520a86adee9a4d4f553, where you see e.g. that pandas went from 1.5.3 to 2.2.3.

tcompa commented 1 month ago

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 commented 1 month ago

Last comment: would it make sense to also address https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/821 here?

jluethi commented 1 month ago

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 commented 1 month ago

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).

@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 commented 1 month ago

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 commented 1 month ago

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 (https://github.com/astropy/astropy/pull/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.