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

Rework minimum requirements check #415

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Rework minimum requirements check #415

merged 1 commit into from
Apr 7, 2023

Conversation

gadomski
Copy link
Member

@gadomski gadomski commented Apr 6, 2023

Description:
This simplifies things a bit (specifically, removes requirements-min.txt which felt a bit hackey). I explicitly don't handle non >= requirement specifications ... we'll cross that bridge when we get there?

I don't think these need a changelog.

PR checklist:

  • Code is formatted (run scripts/format).
  • Code lints properly (run scripts/lint).
  • Tests pass (run scripts/test).
  • Documentation has been updated to reflect changes, if applicable.
  • Changes are added to the CHANGELOG.

@gadomski gadomski requested a review from jpolchlo April 6, 2023 19:33
@gadomski gadomski self-assigned this Apr 6, 2023
Copy link
Contributor

@jpolchlo jpolchlo left a comment

Choose a reason for hiding this comment

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

While attempting to install into a clean virtual environment, there was a complaint that packaging was missing when I attempted to simply run ./scripts/install-min-requirements. Is this script is just intended for use in the CI? I see that requirements-dev.txt includes packaging, and those requirements are pulled in ahead of running this script. However, everything works via pip install. I'm confused as to the function of this script, and if it is not meant for developers, should it live in scripts? Also, the scripts directory was supposed to be consistent with Scripts to Rule Them All, and it might be better to stick to that convention by calling this script bootstrap.

This "request changes" response is provisional. I'm happy to convert to an "approve" with just a few words of explanation.

@gadomski
Copy link
Member Author

gadomski commented Apr 7, 2023

Thanks for the review!

While attempting to install into a clean virtual environment, there was a complaint that packaging was missing when I attempted to simply run ./scripts/install-min-requirements

I'll add a doc note to the top of script explaining that you need to install the dev dependencies first.

Is this script is just intended for use in the CI?

Generally, yeah, though you could use it locally too if you want.

I'm confused as to the function of this script, and if it is not meant for developers, should it live in scripts?

Why not? It's a script :-)

Also, the scripts directory was supposed to be consistent with Scripts to Rule Them All, and it might be better to stick to that convention by calling this script bootstrap.

That's not really documented anywhere in this repo, and we've had the practice of adding other scripts to scripts/ in related projects (e.g. https://github.com/stactools-packages/noaa-cdr/blob/main/scripts/create_examples.py). It's not really a bootstrap script, either -- rather, it's intended as a tool to help ensure our library continues to work w/ the floored versions of its dependencies (which is sort of hard to do, e.g. pypa/pip#8085). For context, I did a writeup of the original setup here: https://www.gadom.ski/2022/02/18/dependency-protection-with-python-and-github-actions.html.

@gadomski gadomski force-pushed the min-check branch 2 times, most recently from db55715 to 1683a1f Compare April 7, 2023 14:05
@jpolchlo
Copy link
Contributor

jpolchlo commented Apr 7, 2023

I wonder if you might include a link to this blog post as context as a comment in the min requirements script? It's good context, and might be valuable to others who stumble in to class late (like me). It clears up the function of what's being done here, and will probably dissuade others from naively attempting to use this script in a more general fashion.

@gadomski gadomski enabled auto-merge (rebase) April 7, 2023 15:57
@gadomski gadomski merged commit d5c3f35 into main Apr 7, 2023
@gadomski gadomski deleted the min-check branch April 7, 2023 16:12
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.

2 participants