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

Ensure that the dll tests clean up after themselves #525

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

llimeht
Copy link
Contributor

@llimeht llimeht commented Oct 25, 2022

One of the model tests (direct_model.py::test_reparameterize) leaves behind stray files from the compiled DLLs, this PR tries to cleanup.

sasmodels/direct_model.py Outdated Show resolved Hide resolved
try:
os.remove(derived_model.dllpath)
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about why you need the try/except I realize it's because you may not have a dll path associated with the kernel, if it is a cuda kernel for example. Which begs the question of why dllpath is part of the "public" interface. We should probably rename it to _dllpath in the kerneldll.py so that you can't do this operation.

Instead we should have a clean method as part of the kernel API for removing the cache for one-offs like this, and this method can reference _dllpath.

Feel free to move this action to a new "clean" method, or merge this as is and add a ticket to address the technical debt.

Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

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

Approving so that you can merge as is or reduce technical debt by moving the action to a "clean" method.

@wpotrzebowski wpotrzebowski merged commit de76c3e into SasView:master Oct 26, 2022
@llimeht llimeht deleted the tmp/model-cleanup branch October 27, 2022 00:42
@llimeht
Copy link
Contributor Author

llimeht commented Oct 27, 2022

Cache-invalidation and cleanup are always complicated. It's not entirely clear to me what the correct behaviour is for the sasmodels cache: what gets cached, what is "prepopulated" into the cache, what is not cached, what cleans out the cache as it grows forever...

Figuring out the use cases and the requirements might be a good thing for a separate discussion.

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