-
Notifications
You must be signed in to change notification settings - Fork 240
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
Try to figure out where/how to insert the cache here #740
Conversation
@ebolyen is this along the lines of what we want? It seems to be doing what we wanted. This isn't the part of the framework I'm most familiar with. |
Almost, I would lift this logic up to Is the process pool the best place? It seems kind of reasonable, although it's looking less like a pool every day. The automated cleanup is a nice feature of it. We should also test this with your parsl situation with the provenance path not showing up. My hunch is it will still work since there's a flufl lock, but worth confirming before we break everything. |
Yeah while we were messing with that I was wondering if this functionality would resolve that |
Ok so we do want to put this in |
Something like how it is now and stop passing in prefix? I had figured this would bork things pretty bad, but maybe not. |
Yep, in theory that's all we should need to do. |
qiime2/core/path.py
Outdated
|
||
cache = get_cache() | ||
kwargs['prefix'] = os.path.join(cache.process_pool.path, | ||
'q2-%s-' % cls.__name__) |
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.
Let's put this under a tmp/
directory in the process pool (creating if needed). Maybe there should be a helper on the cache itself to return the temp directory?
This does not work how I thought it did at all. The actual path is one of the args, and the prefix doesn't seem to do much if anything when passed up to pathlib.Path |
@ebolyen I think putting the cache usage in |
@@ -92,14 +92,20 @@ def _destruct(cls, path): | |||
else: | |||
os.unlink(path) | |||
|
|||
def __new__(cls, dir=False, **kwargs): |
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.
Where there any callers of this? I swear we set prefix
in a lot of places for stuff like q2-provenance
... although come to think of it, cache has probably eroded out most of those callsites, but there may still be a few left.
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.
Only existing use is
Line 23 in c95b12b
self.path = qpath.OutPath( |
The child processes will not clean these tmp dirs out when they exit because their termination does not call exit funcs. |
This may actually be useful for #790 |
Superseded by #790 |
Closes #730