Add taskvine temp/url handling to parsl/taskvine executor #3355
Conversation
|
@colinthomas-z80 this is failing on one of the |
colinthomas-z80
force-pushed
the
master
branch
2 times, most recently
from
8bcfe4a
to
434f804
Compare
|
@benclifford This is working now on my end. Interested to hear any thoughts you have. This is only regarding temp files fyi, the task grouping we discussed will be another endeavor. It seems now that we must declare the stub staging provider we will have to explicitly state all other provider types. Maybe the stub staging provider should be hard-coded in the manager? I'm not sure if that would make a difference though |
| @@ -363,6 +366,7 @@ def submit(self, func, resource_specification, *args, **kwargs): | |||
|
|
|||
| # Also consider any *arg that looks like a file as an input: | |||
| input_files += [self._register_file(f) for f in args if isinstance(f, File)] | |||
| logger.debug(f"registered input files {input_files}") | |||
There was a problem hiding this comment.
for debug logs in Parsl, @khk-globus and I have been pushing on a style that defers string interpolation until the log system has decided if it really wants to render that message - often that's not the case and rendering can be expensive. that format looks like this:
parsl/parsl/channels/local/local.py
Line 58 in 64e163c
rather than using Python-level string formatting (either f-strings or "".format(
parsl/executors/taskvine/manager.py
Outdated
| elif "https://" in filename or "http://" in filename: | ||
| return m.declare_url(filename, cache=cache, peer_transfer=True) | ||
| else: | ||
| return m.declare_file(filename, cache=cache, peer_transfer=True) |
There was a problem hiding this comment.
its a bit ugly that this isn't somehow using the parsed URL form of Parsl files to check the namespace, rather than doing ad-hoc basic URL parsing in this if statement... this is switching on the URI scheme only, I think, with the intention that it works like, for example, StubStaging.can_stage_* I think?
There was a problem hiding this comment.
It is a bit redundant compared to what the staging provider does, but there is not a clear pass-through to declare our vine_file objects through the staging provider. I made a change here which brings the protocol/scheme through to this spot where we can switch on that, a little nicer than url string comparisons.
parsl/tests/configs/taskvine_ex.py
Outdated
|
|
||
|
|
||
| def fresh_config(): | ||
| return Config(executors=[TaskVineExecutor(manager_config=TaskVineManagerConfig(port=9000), | ||
| worker_launch_method='factory')]) | ||
| worker_launch_method='factory', | ||
| storage_access=[FTPInTaskStaging(), StubStaging(), NoOpFileStaging(), ZipFileStaging()])]) |
There was a problem hiding this comment.
this drops off the default HTTP provider, so it's testing that HTTP works via this new mechanism not the other Parsl mechanisms. that's good.
Maybe you can drop off NoOpFileStaging too? (NoOpFileStaging really means "use the shared filesystem" staging, and I think taskvine in general is trying to manage those files better than the shared filesystem can?)
or at least, what breaks if you take away NoOpFileStaging here?
There was a problem hiding this comment.
i think it would be good to add a taskvine specific local test case that validates that taskvinetemp: URLs work (at least that the file contents get moved around correctly) when used with the TaskVineExecutor - seeing as that's the core functionality of this PR
There was a problem hiding this comment.
We definitely want to keep the capability of using the shared filesystem. Removing NoOp staging fails some basic tests staging out local files. I don't see any reason at the moment to re implement local file handling through the new staging provider.
| @@ -215,9 +218,9 @@ def __create_data_and_logging_dirs(self): | |||
|
|
|||
| # Create directories for data and results | |||
| log_dir = os.path.join(run_dir, self.label) | |||
| self._function_data_dir = os.path.join(run_dir, self.label, "function_data") | |||
| self._function_data_dir = os.path.join("/tmp/function_data/", self.label, str(uuid.uuid4())) | |||
There was a problem hiding this comment.
is this a change to say "taskvine is going to manage movements of all these files off this local temporary directory"? if so, is that something that works independently of this PR #3355? (eg. if you did it as a different PR, is that what would happen without any other changes?)
There was a problem hiding this comment.
This change is not in the scope of this PR. I can go ahead and separate the two. This will work independently, and potentially be a benefit to others.
While I was developing this temp file effort I discovered my testing application was writing thousands of function data files to the shared file system, so it got thrown in here for the time being.
There was a problem hiding this comment.
ok, then I think this definitely makes sense to turn into its own PR.
I think you need to consider who owns /tmp/function_data on a shared submit host (it's probably not you, but the first user who ever ran taskvine on this machine) - so maybe this direction should be something like /tmp/parsl-taskvine-{username} so both the guilty user and the guilty software are clearly identified.
|
I brushed up on the urlparse documentation and found that netloc, is indeed supposed to give the "host" part of a url string. Why in most cases it seems to get filled out with the terminal filename I am not certain, but I will start using local_path anyway. |
outside of the parsl/python context, I've seen that from parsing |
Description
Using the stub staging provider in the taskvine executor will give control of staging https files to taskvine. Declaring a file with the protocol
taskvinetemp://will also utilize the temporary/intermediate data features of taskvine.Type of change