-
Notifications
You must be signed in to change notification settings - Fork 274
set_variant(): try next requested variant if one fails to import #1522
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! I've had it on my backlog to have mi.variants()
only report valid variants, but there's one nasty problem: if that logic is ever wrong/broken, we'll be skipping hundreds of tests on the CI. Keeping this fallback logic in set_variant()
shouldn't ever mess with how we've setup the test suite 👍
ff60350
to
4504654
Compare
If the user specifies several variants in order: mi.set_variant("cuda_ad_rgb", "llvm_ad_rgb") and they are all compiled (available) but some fail to import, then keep trying with the next requested variant instead of throwing an exception. This could happen e.g. when requested a CUDA variant with CUDA installed but no GPU available, or when LLVM is not installed.
+ remove unnecessary borrow.
240eebb
to
a4dcf11
Compare
Thanks for the review @njroussel and @wjakob! Currently, both on # CUDA_VISIBLE_DEVICES= python ./test_failed_variant.py
import mitsuba as mi
if __name__ == "__main__":
mi.set_log_level(mi.LogLevel.Debug)
try:
mi.set_variant("cuda_ad_rgb")
except ImportError as e:
print("Failed to set variant")
print("Current variant:", mi.variant())
# Still leaks if we then set a valid variant
mi.set_variant("llvm_ad_rgb")
print("Current variant:", mi.variant())
it's unrelated to this PR, though. Edit: hopefully fixed with 4306f55 |
When importing a variant, immediately try initializing the backend. Without this change, initialization could fail much later, in `color_management_static_initialization()`. By that point, many other things have been loaded, which led to reference leaks to various types and functions, even if the import error was properly handled.
Pinging this PR, would it be okay to merge? The latest commit (4306f55) avoids partial initialization of variants when a backend turns out to be unavailable, which used to result in leaks (even before this PR). |
LGTM! Thanks for looking at the leaks too :) |
Description
If the user specifies several variants in order, e.g.:
and they are all compiled (available) but some fail to import, then keep trying with the next requested variant instead of throwing an exception.
This could happen e.g. when requested a CUDA variant with the NVIDIA driver installed / CUDA available but no GPU available, or when LLVM is not installed.
Testing
On a machine with the NVIDIA driver installed,
CUDA_VISIBLE_DEVICES= python -c 'import mitsuba as mi; mi.set_variant("cuda_ad_rgb", "llvm_ad_rgb"); print(mi.variant())'
Old behavior: raises
ImportError
and exits.New behavior: fails to load
cuda_ad_rgb
, but correctly goes to the second choicellvm_ad_rgb
.The old behavior can still be obtained by requesting a single variant at a time (no fallback):
mi.set_variant("cuda_ad_rgb")
Checklist
cuda_*
andllvm_*
variants. If you can't test this, please leave below