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

🔧 FIX: v2.5.2 requirements included #1598

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Conversation

daquinteroflex
Copy link
Collaborator

Ahhhhhhhhh this was because I was working back them from the 2.4 defacto develop when I started rather than the pre/2.5 I think since I branched out incorrectly and migrated from there. This is why there is the mismatch in the translation of the requirements to pyproject.toml . I'm just judging by the https://github.com/flexcompute/tidy3d/blob/v2.5.2/requirements/web.txt vs https://github.com/flexcompute/tidy3d/blob/v2.6.0/pyproject.toml

@daquinteroflex daquinteroflex changed the title wrench: FIX: v2.5.2 requirements included 🔧 FIX: v2.5.2 requirements included Apr 10, 2024
@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Apr 10, 2024

Note that this PR is correct, it errored because pandas just released a new version 20 mins ago and the new poetry serveer has not fully reconciled with pypi I think. Should involve just running the aciton again.

EDIT: I've only updated the poetry lock up to the required dependencies now, whilst they sort it out.

@daquinteroflex daquinteroflex force-pushed the dario/fix_versions branch 2 times, most recently from 27d7c93 to ac8954f Compare April 11, 2024 07:57
pyproject.toml Outdated
h5netcdf = "1.0.2"
h5py = "^3.0.0"
rich = "<12.6.0"
numpy = "<2"
matplotlib = "*"
shapely = "^2.0"
shapely = ">=2.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between "^" and ">="?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually has different meanings according to how it is used: https://python-poetry.org/docs/dependency-specification/#caret-requirements

It means the higher minor version for a given major version in this context >=2, <3. Whereas >= just means the highest version from this point >=2 including 3.

Copy link
Collaborator Author

@daquinteroflex daquinteroflex Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I changed it to match this: https://github.com/flexcompute/tidy3d/blob/v2.5.2/requirements/basic.txt#L10C1-L10C13

since there were no further PRs on that I found and #642

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, actually ^ does make a lot of sense, I think we maybe just didn't know about it (or it is not supported in requirements.txt file?). Back when shapely introduced v2, we had something like >= 1.8; <2.0 in the file. So maybe ^ is better now actually? :) This can be done either here or in the 2.7 change...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea probably we should keep the ^, and even apply it to things like numpy, shapely, importlib-metadata.

Maybe best for the 2.7 change (with the less restrictive versions) though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that sounds good, will make another PR for 2.7 on this

@tylerflex
Copy link
Collaborator

Note: for 2.7 we should also probably relax the requirements for rich, in addition to these ones.

@momchil-flex momchil-flex merged commit edcf220 into develop Apr 12, 2024
13 checks passed
@momchil-flex momchil-flex deleted the dario/fix_versions branch April 12, 2024 17:36
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