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

Typeguard 4.3.0 #97

Conversation

ChristianWirthContinental
Copy link
Contributor

@ChristianWirthContinental ChristianWirthContinental commented Jul 11, 2024

Fixes #84 by updating typeguard to 4.X

This is important because the typeguard requirement is in conflict with ydata-profiling: https://github.com/ydataai/ydata-profiling/blob/cc7b9b36364fdcb6db6a23db5d83ad3c6e2e3c8f/requirements.txt#L23 - a commonly used EDA package.
This, in turn, prevents updates to pandas and numpy.

All unittests passed

setup.py Outdated Show resolved Hide resolved
@tmct
Copy link

tmct commented Jul 18, 2024

Hang on a minute, this is only used in tests? We can move this from install_requires to being a dependency of only the test environment!

And - it looks like >=3.1,<5 might be a nice balance, given that the commented out code here wants to use that minimum

@tmct
Copy link

tmct commented Jul 18, 2024

Probably easier to describe that by making the change - what do you think? #98

(And if it's a test requirement, we don't have to over think the permissiveness of the lower bound!)

@ChristianWirthContinental
Copy link
Contributor Author

Probably easier to describe that by making the change - what do you think? #98

(And if it's a test requirement, we don't have to over think the permissiveness of the lower bound!)

I think your solution is good, as this fully resolves the conflict also for the future (as long as one is not doing testing).
However, i'm fine with both PRs and just want to see the problem resolved 😄

@ChristianWirthContinental
Copy link
Contributor Author

Moving forward with this or or #98 would be of great help, as it blocks some sever downstream issues. Anything preventing this from the next steps? (@gpleiss ?)

@tmct
Copy link

tmct commented Aug 12, 2024

We are also eager to see either change made, please. (In the meantime we have been using uv's dependency overrides to create functioning environments.)

@gpleiss
Copy link
Member

gpleiss commented Aug 13, 2024

@ChristianWirthContinental @tmct the locked version of typeguard was intentional (but poorly documented in the code). Newer versions of typeguard aren't compatible with jaxtyping. See:

It's worth noting that typeguard versioning has caused previous issues. As a workaround, we can require the specific locked version of typeguard in our testing code (where we need the specific version for jaxtyped tests) but we don't have to lock down typeguard for the main package dependencies (where jaxtyping is strictly used for annotations only).

@gpleiss gpleiss merged commit 62496b6 into cornellius-gp:main Aug 13, 2024
6 checks passed
@tmct
Copy link

tmct commented Aug 13, 2024

Thank you!

n.b. if you ever upgrade your min version of jaxtyping beyond I think 0.2.22, as things stand I think we'd hit the same problem again through transitive dependencies being tightened... So hopefully jaxtyping can be made compatible before then!

@ChristianWirthContinental
Copy link
Contributor Author

@gpleiss Thank you very much! If you don't mind releasing this, i would try to get the new version into botorch to resolve the version conflicts in the dependency chain.

@gpleiss
Copy link
Member

gpleiss commented Aug 15, 2024

Done

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.

Upgrade required typeguard version
3 participants