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

Too strict dependency pinning #1015

Closed
kmuehlbauer opened this issue Sep 29, 2023 · 17 comments
Closed

Too strict dependency pinning #1015

kmuehlbauer opened this issue Sep 29, 2023 · 17 comments

Comments

@kmuehlbauer
Copy link
Collaborator

The strict dependency declarations here:

https://github.com/conda-forge/wetterdienst-feedstock/blob/d7197c9d4b9bfa6dca1ed6629ea10d73d1570c53/recipe/meta.yaml#L25

which are based on those here:

wetterdienst/pyproject.toml

Lines 101 to 120 in e5230da

[tool.poetry.dependencies]
python = "^3.9,!=3.9.7,<3.12"
aenum = "^3.0"
aiohttp = "^3.8.1"
backports-datetime-fromisoformat = "^2.0.0"
beautifulsoup4 = "^4.9"
cachetools = "^5.2"
click = "^8.0"
click-params = "^0.4.1"
cloup = "^3.0.1"
deprecation = "^2.1"
diskcache = "^5.4.0"
environs = "^9.4.0"
fsspec = "^2023.01"
lxml = "^4.9.1"
measurement = "^3.2"
numpy = "^1.22"
pandas = "^2.0"
Pint = "^0.17"

have a massive impact on installing wetterdienst in existing environments or alongside major packages of the scientific python stack.

Currently the strict declaration of Pint = "^0.17" (already at 0.22) doesn't allow to install alongside xarray 2023.09.0 (which requires 0.19 at least).

Is it really necessary to have these strict version pinning on the upper bound for so many packages? Would it be possible just pin on lower bound?

@amotl
Copy link
Member

amotl commented Sep 29, 2023

Dear Kai,

thanks for reporting. Wetterdienst should be a good citizen of the packaging ecosystem, so we should look into relaxing that constraint.

I am a bit hesitant on pinning on the lower bound in general, because, well, that does not protect against breaking changes, specifically with dependencies in their 0.x cycles, because we can't look into the future.

In fact, I am asking myself why Dependabot did not submit a corresponding suggestion to upgrade the Pint package here. Maybe the dependencies of the feedstock package (meta.yaml) went out of sync, and need another round of (separate) maintenance?

With kind regards,
Andreas.

P.S.: I didn't look into the details yet, so I was just writing down what came to my mind about this.

@amotl
Copy link
Member

amotl commented Sep 29, 2023

  1. Dependency definition is Pint = "^0.17" at https://github.com/earthobservations/wetterdienst/blob/v0.60.0/pyproject.toml#L120.
  2. Pint 0.22 is available at https://pypi.org/project/Pint/.
  3. Dependabot did not send a suggestion to upgrade?
  4. Why?

@kmuehlbauer
Copy link
Collaborator Author

Thanks Andreas, much appreciated. It's not only Pint, but also numpy which is stuck at 1.22 and maybe others. Maybe some misconfiguration of dependabot?

@amotl
Copy link
Member

amotl commented Sep 29, 2023

All right, I also wanted to ask if you have more candidates to care about. Thanks! About NumPy, there is GH-1009. Not sure why the feedstock says it is different. @xylar did a great amount of work there, supporting us to get things right (thanks again!), maybe it needs a refresh/review?

@amotl
Copy link
Member

amotl commented Sep 29, 2023

Indeed, numpy = "^1.22" it is what's in our package metadata here. We need to bump it.

-- https://github.com/earthobservations/wetterdienst/blob/v0.60.0/pyproject.toml#L118

@amotl
Copy link
Member

amotl commented Sep 29, 2023

Oh my. Looking at those patches recently submitted by Dependabot, it looks like it only bumps versions in poetry.lock. Well, there we have it. How is this supposed to work? ;]

-- https://github.com/earthobservations/wetterdienst/pull/1009/files
-- https://github.com/earthobservations/wetterdienst/pull/1010/files

@amotl
Copy link
Member

amotl commented Sep 29, 2023

versioning-strategy: lockfile-only . That must have been accidental, but was probably instrumental for a few dependency woes we had been going on.

- package-ecosystem: "pip" # See documentation for possible values
directory: "/" # Location of package manifests
schedule:
interval: "monthly"
versioning-strategy: lockfile-only

@amotl
Copy link
Member

amotl commented Sep 29, 2023

GH-1016 will resolve that problem, but we will need to do some manual version bumping work beforehand. Currently upgrading Poetry on my machine, because, well, dependency staleness is everywhere.

==> Upgrading poetry
  1.4.2 -> 1.6.1

I am still on Catalina, that means Homebrew needs to actually compile stuff (needs a fresh python@{3.9,3.10,3.11} whatever). Feels a bit like Gentoo, but at least we don't suffer from spindle disks any longer. ;] I may continue on that later this day.

@dostuffthatmatters
Copy link

dostuffthatmatters commented Sep 29, 2023

Hi @amotl,

thank you very much for your work on this library! I really enjoy using it.

All right, I also wanted to ask if you have more candidates to care about.

I noticed that polars is still pinned at ^0.16 whereas its library releases are already at 0.19.

Best,
Moritz

@amotl
Copy link
Member

amotl commented Sep 29, 2023

Thanks Moritz. I noticed that as well. Starting with GH-1017, the project metadata will probably need a few more subsequent patches, where polars will also be bumped.

@amotl
Copy link
Member

amotl commented Sep 29, 2023

Hi again,

I've just tried to bump polars, but there is a problem with version 0.17 already, see 1.

pip install "polars<0.18" --upgrade
pytest tests/core/timeseries/test_io.py -k test_export_duckdb

👀

-  datetime.datetime(1939, 7, 26, 0, 0),
?                      ^      ----
+  datetime.datetime(1969, 9, 27, 0, 0),
?                      ^   ++++

Because it doesn't work without further ado, I am adding 04b2d1f, which will effectively not change anything on this matter. We will have to humbly ask @gutzbenj about it, to adjust the code correspondingly, when possible. 🙏

With kind regards,
Andreas.

P.S.: GH-1026 attempts to go straight to polars 0.19. Maybe directly working on supporting this version is applicable, instead of bumping through 0.17 and 0.18 first. I don't think we need to support multiple versions here.

Footnotes

  1. https://github.com/earthobservations/wetterdienst/actions/runs/6357013086/job/17267515343?pr=1018#step:8:5078

@amotl
Copy link
Member

amotl commented Sep 30, 2023

Those are the two major leftovers after bumping all the other dependencies to their most recent versions. Maybe you can add corresponding adjustments to make them being accepted?

@kmuehlbauer
Copy link
Collaborator Author

I can take care of the wradlib changes.

@amotl
Copy link
Member

amotl commented Oct 6, 2023

Hi again,

thank you so much for your support here, @kmuehlbauer, @gutzbenj, and @xylar.

After refreshing the dependencies on the feedstock package, and updating the corresponding release patch accompanying version 0.61.0, we will need to resolve an issue about the eccodes package, where the Anaconda Buildbot complains on behalf of its Azure Pipelines CI run.

With kind regards,
Andreas.

@gutzbenj
Copy link
Member

gutzbenj commented Nov 6, 2023

Dear all,

can we close this issue given the new set of requirements rules?

@amotl
Copy link
Member

amotl commented Nov 6, 2023

Hi again,

if Kai is satisfied with the outcome now, I think it is safe to close the issue.

Cheers,
Andreas.

@kmuehlbauer
Copy link
Collaborator Author

Great work. Thanks a bunch!

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

No branches or pull requests

4 participants