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

ENH: automatically load external frontends in load() #4285

Merged
merged 6 commits into from
Jan 10, 2023

Conversation

Xarthisius
Copy link
Member

PR Summary

This PR modifies the behavior of yt.load to auto-include subclasses of Dataset from external, installed packages. External packages must define an entry point in yt.frontends linked to the Dataset subclass (see docs for an example).

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@Xarthisius Xarthisius added the enhancement Making something better label Jan 8, 2023
@Xarthisius Xarthisius force-pushed the plugin_frontend branch 3 times, most recently from 2a23551 to 4894cfa Compare January 8, 2023 21:21
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I had no idea this was possible, but I love it, thank you !

doc/source/developing/extensions.rst Outdated Show resolved Hide resolved
yt/loaders.py Outdated Show resolved Hide resolved
yt/loaders.py Outdated Show resolved Hide resolved
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

additional pointers. I haven't studied why the test is failing on Windows yet

yt/tests/test_external_frontends.py Outdated Show resolved Hide resolved
yt/loaders.py Outdated Show resolved Hide resolved
yt/loaders.py Show resolved Hide resolved
yt/loaders.py Outdated Show resolved Hide resolved
@Xarthisius
Copy link
Member Author

How to fix windows is beyond me... Why answer tests fail (with blank differences) is also beyond me. I think I addressed everything else.

@neutrinoceros
Copy link
Member

How to fix windows is beyond me...

Am I right to assume fixing #4286 will not resolve the problem here ?
I will try to find a solution here.

Why answer tests fail (with blank differences) is also beyond me.

This is just flakiness, do not worry about it: it's been like this for months now, and we have a solution in #4122

@Xarthisius
Copy link
Member Author

How to fix windows is beyond me...

Am I right to assume fixing #4286 will not resolve the problem here ? I will try to find a solution here.

I pushed the change, let's see.

@neutrinoceros
Copy link
Member

Well played, looks like this now fixes #4286 , right ?
My two unresolved threads still stand, is it alright if I push one commit to add test cleanup, or would you prefer that I write the fixture as a comment ?

@Xarthisius Xarthisius changed the title Automatically load external frontends in load() ENH: automatically load external frontends in load() Jan 9, 2023
@Xarthisius
Copy link
Member Author

My two unresolved threads still stand, is it alright if I push one commit to add test cleanup, or would you prefer that I write the fixture as a comment ?

You can push directly.

@neutrinoceros
Copy link
Member

cool. I'll do this in the next couple hours

@neutrinoceros
Copy link
Member

I will double check that it works as is for yt_idefix

neutrinoceros
neutrinoceros previously approved these changes Jan 9, 2023
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

It works perfectly with yt_idefix, thanks again for starting this !

chrishavlin
chrishavlin previously approved these changes Jan 10, 2023
Copy link
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

this is great!

I do think it will marginally increase the chance of having plugin loading conflicts. Since previously you had to explicitly import the plugin, you'd never end up with two external frontends identifying a file as valid. But now if a user happens to have installed two external entrypoint-enabled frontend plugins that both identify a file as valid they'll get an YTAmbiguousDataType error. But the benefit here far outweighs the slim chance of encountering that IMO so I don't think it's worth worrying about, but I thought it was worth mentioning.

doc/source/developing/extensions.rst Outdated Show resolved Hide resolved
@neutrinoceros
Copy link
Member

But now if a user happens to have installed two external entrypoint-enabled frontend plugins that both identify a file as valid they'll get an YTAmbiguousDataType error. But the benefit here far outweighs the slim chance of encountering that IMO so I don't think it's worth worrying about, but I thought it was worth mentioning.

That's correct, although it's nice to think that this will not be a deadend situation even then thanks to yt.load's hint keyword argument !

@Xarthisius Xarthisius dismissed stale reviews from chrishavlin and neutrinoceros via bb013b7 January 10, 2023 18:31
neutrinoceros
neutrinoceros previously approved these changes Jan 10, 2023
Co-authored-by: Chris Havlin <chris.havlin@gmail.com>
@neutrinoceros
Copy link
Member

pre-commit.ci autofix

@neutrinoceros
Copy link
Member

marking this PR for backport but I'll only cherry pick b276292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants