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

rewrite the min_deps_check script #9754

Merged
merged 17 commits into from
Nov 10, 2024
Merged

rewrite the min_deps_check script #9754

merged 17 commits into from
Nov 10, 2024

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Nov 8, 2024

This uses py-rattler and rich to re-implement the min_deps_check.

py-rattler allows us to fetch all versions at the same time, and we also get rid of the conda dependency (additionally, since rattler.Gateway.query is async and implemented in Rust, the script executes almost immediately).

rich, in turn, makes the output easier to read and understand (and more colorful, too!)

  • delete the old ci/min_deps_check.py script

Sorry, something went wrong.

@headtr1ck
Copy link
Collaborator

Looks colorful, nice.

Some columns are a bit squished, so you cannot really read what's going on

@TomNicholas
Copy link
Member

Shouldn't you also have deleted the original ci/min_deps_check.py file?

@keewis
Copy link
Collaborator Author

keewis commented Nov 8, 2024

Some columns are a bit squished, so you cannot really read what's going on

It properly works locally... my guess is that rich doesn't quite understand the width given by the GH actions runner. I'll look into that.

Shouldn't you also have deleted the original ci/min_deps_check.py file?

Obviously, I was just in a hurry when I created the PR.

@keewis
Copy link
Collaborator Author

keewis commented Nov 8, 2024

my guess is that rich doesn't quite understand the width given by the GH actions runner.

looks like the issue is rather that GH strips some control characters necessary to format the tables (see Textualize/rich#3500). I don't know where to look for a workaround for that, but after enforcing a larger line width the table is just barely readable now.

@keewis
Copy link
Collaborator Author

keewis commented Nov 8, 2024

well, looks like removing background colors did the trick...

Other than that, it looks like I'm not good at styling things. If anyone has ideas for colors (rgb) for 4 different styles:

  1. section headers (the "version summary" / "warnings" text at the left)
  2. good pin
  3. version is too old (pin can be bumped)
  4. version is too new (pin should be downgraded)

I'd be happy to take suggestions.

Right now, those are (rgb without the modifiers):

  1. "bold red1" (#ff0000)
  2. "bold dim green3" (#00d700)
  3. "bold orange3" (#d78700)
  4. "bold red1" (#ff0000)

@headtr1ck
Copy link
Collaborator

On GitHub Mobile it's all messed up
IMG_20241109_101743.jpg

But I guess we don't care about that, lots of things don't work in this app.

I think the colors are fine.

@dcherian
Copy link
Contributor

dcherian commented Nov 9, 2024

Very nice! This is a great improvement. I say we merge whenever you're ready.

@keewis
Copy link
Collaborator Author

keewis commented Nov 9, 2024

I just pushed a couple of commits that move the styles to a single location within the formatter, and also makes it use explicit objects instead of style strings. In theory we could also create a Theme object in main and use names in the formatter (or we could have it select the theme based on the CI environment variable), but I will leave that to anyone who is interested in fiddling with colors.

Another point I forgot to mention is that even though py-rattler is really fast, we call the script once per min-deps environment, i.e. two times. In theory the script can process both at the same time (and thus avoid fetching the relevant repodata files multiple times), but I guess that doesn't help as much if we want to easily pinpoint the failing env by having the job fail in different steps. If anyone has ideas on how to improve on that, feel free to modify the script / job.

Both changes are optional, though, so as far as I'm concerned this is ready to be merged!

@dcherian dcherian merged commit c425779 into pydata:main Nov 10, 2024
29 checks passed
@keewis keewis deleted the min-deps branch November 10, 2024 01:07
@mathause
Copy link
Collaborator

Do you think it is possible to turn this into a github action? I have several packages where I copy this script over... Maybe others could also profit from this. Not sure if e.g. flox has a min-deps check?

@keewis
Copy link
Collaborator Author

keewis commented Nov 11, 2024

that would certainly be possible, yes. Where would that live? xarray-contrib?

@mathause
Copy link
Collaborator

Given issue-from-pytest-log lives there this might as well.

dcherian added a commit that referenced this pull request Nov 19, 2024
* main: (24 commits)
  Bump minimum versions (#9796)
  Namespace-aware `xarray.ufuncs` (#9776)
  Add prettier and pygrep hooks to pre-commit hooks (#9644)
  `rolling.construct`: Add `sliding_window_kwargs` to pipe arguments down to `sliding_window_view` (#9720)
  Bump codecov/codecov-action from 4.6.0 to 5.0.2 in the actions group (#9793)
  Buffer types (#9787)
  Add download stats badges (#9786)
  Fix open_mfdataset for list of fsspec files (#9785)
  add 'User-Agent'-header to pooch.retrieve (#9782)
  Optimize `ffill`, `bfill` with dask when `limit` is specified (#9771)
  fix cf decoding of grid_mapping (#9765)
  Allow wrapping `np.ndarray` subclasses (#9760)
  Optimize polyfit (#9766)
  Use `map_overlap` for rolling reductions with Dask (#9770)
  fix html repr indexes section (#9768)
  Bump pypa/gh-action-pypi-publish from 1.11.0 to 1.12.2 in the actions group (#9763)
  unpin array-api-strict, as issues are resolved upstream (#9762)
  rewrite the `min_deps_check` script (#9754)
  CI runs ruff instead of pep8speaks (#9759)
  Specify copyright holders in main license file (#9756)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Nov 19, 2024
* main:
  fix html repr indexes section (pydata#9768)
  Bump pypa/gh-action-pypi-publish from 1.11.0 to 1.12.2 in the actions group (pydata#9763)
  unpin array-api-strict, as issues are resolved upstream (pydata#9762)
  rewrite the `min_deps_check` script (pydata#9754)
  CI runs ruff instead of pep8speaks (pydata#9759)
  Specify copyright holders in main license file (pydata#9756)
  Compress PNG files (pydata#9747)
  Dispatch to Dask if nanquantile is available (pydata#9719)
  Updates to Dask page in Xarray docs (pydata#9495)
  http:// → https:// (pydata#9748)
  Discard useless `!s` conversion in f-string (pydata#9752)
  Apply ruff/flake8-simplify rule SIM401 (pydata#9749)
  Use micromamba 1.5.10 where conda is needed (pydata#9737)
  pin array-api-strict<=2.1 (pydata#9751)
  Reorganise ruff rules (pydata#9738)
  use new conda-forge package pydap-server (pydata#9741)
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.

None yet

5 participants