UX: add information about missing optional dependencies as warnings and error messages in yt.load

[neutrinoceros]

PR Summary

Fix #4274
For now I've only tested this manually but it should be relatively straightforward to add tests.

TODO:

• tests
• clean up redundant checks and warnings (warn_h5py and the likes) from _is_valid methods
• implement early returns in _is_valid methods when requirements are missing
• rebase

[chrishavlin]

I'd like to check out the branch and play around with this before hitting the approve button, but here are a few comments and questions in the mean time. Generally looks good to me though!

[neutrinoceros]

FYI @chrishavlin I just forced pushed again to resolve a conflict. I don't intend to touch this branch anymore for now so feel free to play around with it

[neutrinoceros]

I would still be happy to see this in yt 4.2, but it can wait, and it doesn't seem worth blocking either, so I'll remove it from the milestone

[neutrinoceros]

I'll actually go one step further and add the "do not merge" label since it contains new deprecation warnings that need to reflect the first version they appeared in, and it doesn't look like it's going to be 4.2

[chrishavlin]

Thanks for the ping! Should be able to get to this tomorrow!

[chrishavlin]

Ok! had a chance to test this out locally and it's great. I noted one odd quirk with enzo, but this is good to go after the conflicts are resolved IMO.

[chrishavlin]

OK, odd quirk here! I just tried loading IsolatedGalaxy in a fresh environment with a minimal yt install (so no h5py), and you get:

>>> ds = yt.load('IsolatedGalaxy/galaxy0030/galaxy0030')
<stdin>:1: UserWarning: This dataset appears to be of type EnzoDataset, but the following requirements are currently missing: h5py, libconf
Please verify your installation.
yt : [INFO     ] 2023-07-20 11:34:36,033 Parameters: current_time              = 0.0060000200028298
yt : [INFO     ] 2023-07-20 11:34:36,033 Parameters: domain_dimensions         = [32 32 32]
yt : [INFO     ] 2023-07-20 11:34:36,033 Parameters: domain_left_edge          = [0. 0. 0.]
yt : [INFO     ] 2023-07-20 11:34:36,034 Parameters: domain_right_edge         = [1. 1. 1.]
yt : [INFO     ] 2023-07-20 11:34:36,034 Parameters: cosmological_simulation   = 0

Based on the _is_valid here, the fact that the load appears to work (you actually get populated parameters!) would not be new to this PR , so it's actually quite helpful to have that UserWarning show up. But would it make sense to add an error here?

Initially I was gonna suggest adding

        if cls._missing_load_requirements():
            return False

like in the other updated _is_valid methods. But then that would print out the full list of h5py-dependent frontends that are possible. And here we actually know it's enzo.

[neutrinoceros]

I think the paradigm has always been that _is_valid methods are not allowed to throw errors as it would give each frontend the power to break all others. Maybe I'm not understanding your suggestion ?

[chrishavlin]

Not suggesting to throw an error within the method.

Just saying that you could refactor this is_valid to return False if the dependencies are missing, regardless of the filename convention check that it does now. That would result in a unidentified datatype error right away rather than a partially functional ds.

[chrishavlin]

I'm just on the fence on whether it's better have a non functional ds but know that it's Enzo or an error right away and not know it's Enzo.

[neutrinoceros]

oh, I see. The behaviour you're pointing to is not intended ! I must have been testing this with yt.load_sample("IsolatedGalaxy") instead, which behaves differently. Your suggestion makes both loaders behave similarly, which is great, but we also loose the warning... I'll switch to draft while I try to think of a better solution.

[neutrinoceros]

ah, I see now that I replied in a hurry: as you already pointed, the behaviour of the Enzo frontend is weird on the main branch, and my patch only adds a warning to it (which is the point).
I'd prefer to keep this PR's scope limited to adding warnings and details to existing errors, rather than actively changing frontends' behaviour, just to keep things as tidy as possible. But I would merge your suggestion as a standalone patch ! WDYT ?

[chrishavlin]

Ya, that sounds good to me!

[chrishavlin]

Mostly just wanted to bring up this behavior --- even without a follow-up patch, having the warning now is better than the behavior on main!

[neutrinoceros]

conflicts resolved

[neutrinoceros]

this is good to go after the conflicts are resolved IMO.

@chrishavlin ? 👼🏻

[neutrinoceros]

Ah, I forgot to update the deprecation messages (which is why I used the "do not merge" label). I'll do it now and hopefully we can merge then.

[neutrinoceros]

@chrishavlin sorry to ping you again. This is now ready to go !