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

Fix issue #72 Forward Hooks Persist After Destroying FeatureExtractor #91

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

ShunsukeOnoo
Copy link
Contributor

This pull request fixes issue #72 where forward hooks on the encoder persist after destroying the FeatureExtractor. This issue caused unnecessary features to be stored incrementally, consuming RAM if the same encoder was used for another reconstruction. This pull request adds a destructor to the FeatureExtractor class that removes all forward hooks added by it. With these updates, the same encoder can be used for reconstruction multiple times without needing to be reloaded.

@ShuntaroAoki ShuntaroAoki changed the base branch from main to dev July 25, 2024 07:16
@ShuntaroAoki
Copy link
Member

Thank you for the PR. This is great! I'm readly to merge it.
However, there is one concern: this will remove all forward hooks registered before the FeatureExtractor instance. Although that might be very rare situation and we could just ignore it, I'd like to avoid such a side effect as much as possible. Could you fix the code to only remove hooks that were added by the instance, or at least raise a warning to notify users that all hooks are removed?

@ShunsukeOnoo
Copy link
Contributor Author

Thank you for your comments. I modified a bit so that only the forward hooks added by the extractor will be removed. I have also found that my original code didn't work when using layer_mapping, so I fixed it as well.

@ShuntaroAoki ShuntaroAoki merged commit 6075f53 into KamitaniLab:dev Jul 29, 2024
0 of 9 checks passed
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.

2 participants