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

neutrinoceros · 2022-12-29T13:54:50Z

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!

yt/data_objects/static_output.py

yt/frontends/chombo/data_structures.py

yt/frontends/enzo/data_structures.py

yt/frontends/rockstar/data_structures.py

yt/loaders.py

neutrinoceros · 2023-04-14T22:32:48Z

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 · 2023-05-03T20:15:21Z

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 · 2023-05-04T17:44:27Z

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 · 2023-07-19T23:11:46Z

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 · 2023-07-20T16:51:18Z

yt/frontends/enzo/data_structures.py

+    def _is_valid(cls, filename: str, *args, **kwargs) -> bool:
+        return filename.endswith(".hierarchy") or os.path.exists(
+            f"{filename}.hierarchy"
+        )


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.

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 ?

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.

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.

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.

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 ?

Ya, that sounds good to me!

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 · 2023-07-20T17:07:49Z

conflicts resolved

…nd error messages in yt.load

neutrinoceros · 2023-09-12T09:13:28Z

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

@chrishavlin ? 👼🏻

neutrinoceros · 2023-09-12T16:24:01Z

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 · 2023-09-12T19:26:51Z

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

neutrinoceros added UX user-experience enhancement Making something better labels Dec 29, 2022

neutrinoceros force-pushed the ux_missing_load_reqs branch 3 times, most recently from 64e1df1 to 2a7e176 Compare December 29, 2022 13:58

neutrinoceros changed the title ~~UX: add information about missing optional dependencies as warnings and error messsages in yt.load~~ UX: add information about missing optional dependencies as warnings and error messsages in yt.load Dec 29, 2022

neutrinoceros added the deprecation deprecate features or remove deprecated ones label Dec 29, 2022

neutrinoceros force-pushed the ux_missing_load_reqs branch 5 times, most recently from e714947 to a236aef Compare December 30, 2022 13:16

neutrinoceros changed the title ~~UX: add information about missing optional dependencies as warnings and error messsages in yt.load~~ UX: add information about missing optional dependencies as warnings and error messages in yt.load Dec 30, 2022

neutrinoceros force-pushed the ux_missing_load_reqs branch from a236aef to 7a013fc Compare December 30, 2022 13:56

neutrinoceros marked this pull request as ready for review December 30, 2022 14:46

neutrinoceros added this to the 4.2.0 milestone Dec 31, 2022

neutrinoceros force-pushed the ux_missing_load_reqs branch from 7a013fc to 7e03ea6 Compare January 3, 2023 13:38

neutrinoceros added the code frontends Things related to specific frontends label Jan 30, 2023

neutrinoceros force-pushed the ux_missing_load_reqs branch from 7e03ea6 to b680b93 Compare February 6, 2023 19:49

neutrinoceros marked this pull request as draft March 23, 2023 16:54

neutrinoceros marked this pull request as ready for review April 4, 2023 10:29

neutrinoceros force-pushed the ux_missing_load_reqs branch 2 times, most recently from 7c672bf to 55015cb Compare April 14, 2023 17:40

chrishavlin reviewed Apr 14, 2023

View reviewed changes

neutrinoceros force-pushed the ux_missing_load_reqs branch from a286b40 to 41f1323 Compare April 14, 2023 22:31

neutrinoceros removed this from the 4.2.0 milestone May 3, 2023

neutrinoceros added the do not merge this change should not be merged until later label May 4, 2023

chrishavlin previously approved these changes Jul 20, 2023

View reviewed changes

neutrinoceros dismissed chrishavlin’s stale review via fdac07b July 20, 2023 17:07

neutrinoceros force-pushed the ux_missing_load_reqs branch from 41f1323 to fdac07b Compare July 20, 2023 17:07

neutrinoceros force-pushed the ux_missing_load_reqs branch 2 times, most recently from 060a62c to 175fb09 Compare July 20, 2023 17:35

neutrinoceros marked this pull request as draft July 20, 2023 17:43

neutrinoceros marked this pull request as ready for review July 21, 2023 08:47

neutrinoceros force-pushed the ux_missing_load_reqs branch from 175fb09 to 13ff01c Compare July 21, 2023 08:49

neutrinoceros force-pushed the ux_missing_load_reqs branch from 13ff01c to f3ad989 Compare September 9, 2023 19:30

neutrinoceros requested a review from chrishavlin September 9, 2023 19:30

UX: add information about missing optional dependencies as warnings a…

857bc70

…nd error messages in yt.load

neutrinoceros force-pushed the ux_missing_load_reqs branch from f3ad989 to 857bc70 Compare September 9, 2023 19:33

chrishavlin previously approved these changes Sep 12, 2023

View reviewed changes

DEPR: update 'since' argument in new deprecation to yt 4.3

5c2bfb6

neutrinoceros dismissed chrishavlin’s stale review via 5c2bfb6 September 12, 2023 16:26

neutrinoceros removed the do not merge this change should not be merged until later label Sep 12, 2023

neutrinoceros enabled auto-merge September 12, 2023 16:26

chrishavlin approved these changes Sep 12, 2023

View reviewed changes

neutrinoceros merged commit bd29501 into yt-project:main Sep 12, 2023
12 checks passed

neutrinoceros deleted the ux_missing_load_reqs branch September 13, 2023 06:12

neutrinoceros added this to the 4.3.0 milestone Sep 13, 2023

neutrinoceros mentioned this pull request Oct 6, 2023

[ENH] Read Rockstar members #4661

Merged

1 task

neutrinoceros mentioned this pull request Nov 18, 2023

BUG: avoid a noisy self-triggered warning about libconf being a hard dependency to load any enzo data #4746

Merged

This was referenced Oct 18, 2024

UX: better error messages for frontends with missing bindings #3409

Closed

Some examples might require additional dependencies #5026

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

neutrinoceros commented Dec 29, 2022 •

edited

Loading

chrishavlin left a comment

neutrinoceros commented Apr 14, 2023

neutrinoceros commented May 3, 2023

neutrinoceros commented May 4, 2023

chrishavlin commented Jul 19, 2023

chrishavlin left a comment

chrishavlin Jul 20, 2023 •

edited

Loading

neutrinoceros Jul 20, 2023

chrishavlin Jul 20, 2023

chrishavlin Jul 20, 2023 •

edited

Loading

neutrinoceros Jul 20, 2023

neutrinoceros Jul 21, 2023

chrishavlin Jul 21, 2023

chrishavlin Jul 21, 2023 •

edited

Loading

neutrinoceros commented Jul 20, 2023

neutrinoceros commented Sep 12, 2023

neutrinoceros commented Sep 12, 2023

neutrinoceros commented Sep 12, 2023

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

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

Conversation

neutrinoceros commented Dec 29, 2022 • edited Loading

PR Summary

chrishavlin left a comment

Choose a reason for hiding this comment

neutrinoceros commented Apr 14, 2023

neutrinoceros commented May 3, 2023

neutrinoceros commented May 4, 2023

chrishavlin commented Jul 19, 2023

chrishavlin left a comment

Choose a reason for hiding this comment

chrishavlin Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

neutrinoceros Jul 20, 2023

Choose a reason for hiding this comment

chrishavlin Jul 20, 2023

Choose a reason for hiding this comment

chrishavlin Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

neutrinoceros Jul 20, 2023

Choose a reason for hiding this comment

neutrinoceros Jul 21, 2023

Choose a reason for hiding this comment

chrishavlin Jul 21, 2023

Choose a reason for hiding this comment

chrishavlin Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

neutrinoceros commented Jul 20, 2023

neutrinoceros commented Sep 12, 2023

neutrinoceros commented Sep 12, 2023

neutrinoceros commented Sep 12, 2023

neutrinoceros commented Dec 29, 2022 •

edited

Loading

chrishavlin Jul 20, 2023 •

edited

Loading

chrishavlin Jul 20, 2023 •

edited

Loading

chrishavlin Jul 21, 2023 •

edited

Loading