-
Notifications
You must be signed in to change notification settings - Fork 6
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 2024/04 #68
Conversation
Thanks a lot for the quick fix @glatterf42 👍 |
Hello I removed the copied columns and put them back in each map function. From my side this looks good and can be merged! I also updated dask because there is a brand new python3.11 bug. |
pyproject.toml
Outdated
openpyxl = "^3.0.9" | ||
pandas = "~2.1.2" | ||
pandas = "^2.2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the fix not work with pandas 2.1? Seems very tight to depend on a package version that was released less than 2 months ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial trigger for this PR was to remove the version pin of pandas. ~2.1.2
translates to >=2.1.2, <2.2.0
, but climate-processor needs pandas 2.2.0 (if I understand correctly). Updating dependencies in poetry is usually done by running something like poetry add pandas@latest
, which resolves to the latest version and uses the ^
notation, where ^2.2.1
translates to >=2.2.1, <3.0.0
.
In other words, what we have now is simply the default. We could manually lower the requirement to ^2.1.2
, which would allow climate-processor to resolve to the newer versions, too. However, it looks like pandas is releasing v2.2.2 today, so we won't only support the latest version either way. I think it's fine to keep the dependency as it is right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, ixmp4 is now a crucial dependency of pyam and that package is used by many people, so we should be as relaxed as possible - unless there is a good reason to be strict.
See this user feedback IAMconsortium/pyam#840
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I have now used uv's wonderful lowest-direct
dependency resolution to arrive at a requirements.txt file with version pins for python3.10 that are compatible with our project. This process needs as input a requirements.in file, which I generated from our current pyproject.toml by replacing all version specifiers with >=
. At first, I was a bit too radical in keeping only the major version; so if we required a project's 2.1.12 version, I'd keep >= 2.0
in my requirements.in. This produced problems at times and wasn't applicable to projects with versions starting with 0.
. In those cases, I looked at pypi and chose a version more or less at random that is some good six months old.
Next, I created a new venv with python 3.10 and installed all versions that were pinned in the requirements.txt (see below). And the tests are successful :)
So I could now hijack this PR and poetry add
all these dependencies with >=
marks to expand the dependencies we support somewhat in time. For example, we can still support
- pandas 2.0.0
- dask 2023.9.0
- fastapi 0.100.0
and several others. Please let me know what you think, @danielhuppmann
requirements.txt
The versions in here pass our test suite with python 3.10 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meksor Please also let me know what you think and whether we should revert some more internal dependencies to ^
again. The dev, docs, server
groups come to mind as groups that might warrant more stringent dependency limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @glatterf42, this is super-helpful!
From my point of view, we should add a nightly test (running once a week or so) to alert us when a new release of some dependency package breaks something - but given that ixmp4 is now quite stable and used in production by more and more people, better to be lenient with dependencies...
bda164a
to
ca9bc6f
Compare
@meksor please let me know what you think of the dependency changes. |
Has something changed from my last comment 3d ago? |
Yes, we now support some older versions, i.e. several dependencies are now not as strict as they used to be. |
Also, I removed essentially all upper version bounds. |
Ah I see, well if the tests pass its fine with me, but then daniels suggestion for nightly builds seems even more salient now! |
But we can do that in another PR :) |
This PR completes most open dependency updates, most notably removing the pin on pandas < 2.2.0.
Two notes:
So I started tinkering with the options, but I couldn't figure out how to successfully select the grouping columns so that
apply
works on them.groupby(as_index=False, ...)
doesn't seem to have an effect, which might suggest that pandas is treating ourapply
call as a transformation in the background. Simply includingapply.(include_groups=False, ...)
resulted in an error that the'type'
column was missing from the dataframe. Finally, I came across this suggestion: https://stackoverflow.com/a/78100669/23037943 (see in particular the first reply). The suggested workaround copies the column we want togroupby
and thengroup(s)by
the copied column, thus leaving the original open to be passed toapply
as usual. Of course, this is temporarily creating extra data columns which we don't need, so it very much feels like there should be a better solution hiding somewhere. @meksor and @danielhuppmann, please let me know if you have any idea.Apart from this, we probably want to release 0.7.4 after merging this PR to resolve the issue @phackstock had downstream.
FYI @pmussak