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

Detach new_covar_cache to enable JIT tracing of models after fantasization #2605

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SaiAakash
Copy link

This PR deals with #2604 . It's a one-liner where the new_covar_cache is detached so that models which are conditioned on new data/observations using get_fantasy_model can now be JIT traced/exported to TorchScript.

Copy link
Member

@gpleiss gpleiss left a comment

Choose a reason for hiding this comment

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

If we always detach the root then we can't backpropagate through to inputs (necessary for BO applications).

We should only detach the root when detach_test_caches context manager is not on.

@SaiAakash
Copy link
Author

SaiAakash commented Nov 13, 2024

@gpleiss In all other places in GPyTorch, tensors are detached only when the detach_test_caches setting is on. I am wondering why in this case we have to detach when this setting is not on ?

Should it be something like,

if detach_test_caches.on():
        new_covar_cache = new_lt.root_inv_decomposition().root.detach()
else:
        new_covar_cache = new_lt.root_inv_decomposition().root

@jacobrgardner
Copy link
Member

@gpleiss In all other places in GPyTorch, tensors are detached only when the detach_test_caches setting is on. I am wondering why in this case we have to detach when this setting is not on ?

Should it be something like,

if detach_test_caches.on():
        new_covar_cache = new_lt.root_inv_decomposition().root.detach()
else:
        new_covar_cache = new_lt.root_inv_decomposition().root

@SaiAakash Yes, exactly -- we should only detach when the setting is on.

@gpleiss does trace_mode imply detach_test_caches? If no, it probably should I guess?

@SaiAakash
Copy link
Author

@jacobrgardner I have made the requested change. The cache is now detached only when the detach_test_caches setting is on.

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

Successfully merging this pull request may close these issues.

3 participants