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: add API to switch raytracing engine at runtime #4142

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

neutrinoceros
Copy link
Member

PR Summary

This allows to properly define pytest fixtures for switching between "yt" and "embree" ray tracing engine options at runtime (useful in #4122)

Also, a follow up to #3626

@neutrinoceros neutrinoceros added enhancement Making something better tests: running tests Issues with the test setup labels Sep 24, 2022
@neutrinoceros neutrinoceros force-pushed the reusable_raytracing_switch branch 4 times, most recently from 4461e50 to 65ce6d3 Compare September 24, 2022 10:17
@neutrinoceros neutrinoceros marked this pull request as ready for review September 24, 2022 11:06
matthewturk
matthewturk previously approved these changes Sep 24, 2022
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

Nice!

@neutrinoceros neutrinoceros marked this pull request as draft September 24, 2022 11:23
@neutrinoceros
Copy link
Member Author

@matthewturk I just realised I don't need the boolean flag, give me a minute to remove it and then you can set this to auto-merge if you'd like

matthewturk
matthewturk previously approved these changes Sep 24, 2022
@neutrinoceros
Copy link
Member Author

Sorry about the additional iterations, I think it's good now.

@neutrinoceros
Copy link
Member Author

sigh... actually I do need it, otherwise my fixtures just crash pytest

@neutrinoceros neutrinoceros marked this pull request as draft September 24, 2022 11:57
auto-merge was automatically disabled September 24, 2022 11:57

Pull request was converted to draft

@neutrinoceros
Copy link
Member Author

Nevermind. This is usable as is, and validated via #4122

@neutrinoceros neutrinoceros marked this pull request as ready for review September 24, 2022 12:51
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Sep 24, 2022

A problem I failed to see originally here is that "embree" is currently the default ray tracing engine. In the current state, this means yt will raise a warning at startup time unless one of the following conditions is met:

  • embree is correctly installed
  • the parameter is override from the config file

This isn't acceptable.
So here are the options I can think of:

  1. Change the default engine to "yt". This has the downside of breaking users relying on being able to use embree with no additional configuration (other than installing it).
  2. reintroduce a very boolean flag so that we can filter the warning, and use it specifically at initialisation (adds complexity to an already very complex code section)
  3. never warn the set_raytracing_engine("embree") fails

None of these are ideal. I think 1) is the correct thing to do, provided the breaking change clearly mentioned in release notes. Notes on 1):

  • it doesn't force users to start using configuration files since they also get the option of switching engine at runtime.
  • pyembree sadly seems to be kinda discontinued. It's never been trivial to install without conda and even there, conda-forge hasn't received any new binaries for it in 2.5 yrs at this point, which means we can only test it simply on conda + Python 3.8. See https://anaconda.org/conda-forge/pyembree/files

Hopefully this feels acceptable not just to me, I'll let reviewers discuss this point.

@neutrinoceros
Copy link
Member Author

I've now implemented my preferred solution (1)

@neutrinoceros neutrinoceros added this to todo in nose -> pytest migration via automation Sep 30, 2022
@neutrinoceros neutrinoceros moved this from todo to In progress in nose -> pytest migration Sep 30, 2022
@matthewturk matthewturk merged commit c9cb5de into yt-project:main Sep 30, 2022
nose -> pytest migration automation moved this from In progress to Done Sep 30, 2022
@neutrinoceros neutrinoceros deleted the reusable_raytracing_switch branch September 30, 2022 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better tests: running tests Issues with the test setup
Development

Successfully merging this pull request may close these issues.

None yet

2 participants