pydata / xarray

N-D labeled arrays and datasets in Python
https://xarray.dev
Apache License 2.0
3.63k stars 1.09k forks source link

rewrite the `min_deps_check` script #9754

Closed keewis closed 1 week ago

keewis commented 1 week ago

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!)

headtr1ck commented 1 week ago

Looks colorful, nice.

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

TomNicholas commented 1 week ago

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

keewis commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

keewis commented 1 week ago

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!

mathause commented 1 week ago

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 commented 1 week ago

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

mathause commented 1 week ago

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