-
Notifications
You must be signed in to change notification settings - Fork 195
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
TaskVineExecutor: write function data to temp directory #3592
Conversation
I ran in my dev container and I got this surprise.
|
did some strace, and it's looking in this file which contains a decimalised unsigned 32 bit integer representation of signed 32-bit
and then failing after that. The documentation for Python's https://docs.python.org/3/library/os.html#os.getlogin
and in my dev environment that is able to figure out who I am:
|
parsl/executors/taskvine/executor.py
Outdated
@@ -215,7 +216,8 @@ 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") | |||
tmp_dir = os.path.join('/tmp/', f'{self.label}-{os.getlogin()}') | |||
self._function_data_dir = os.path.join(tmp_dir, datetime.now().strftime('%Y%m%d%H%M%S%f'), "function_data") |
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.
This is more a reference for readers of #1122 that review comment for @colinthomas-z80 - here's another instance of using a timestamp as a unique ID that #1122 might want to address
Github Actions testing shows that same OS error - it's not just my wacky dev environment. |
I do recognize the potential issues using datetime. In this instance it is specific to milliseconds so a conflict is unlikely. My rationale was to follow a logging convention in taskvine that will show the logs in a directory listing by order of creation, so it is easier to locate the most recent run. Though these files are rarely useful for a user to inspect themselves. I can change to uuid if you think it conforms better. |
I don't think there's anything to be too concerned about here... mostly I was tagging it for the eventual implementer of #1122 to come fix it up. |
FWIW, this is precisely the purview of the import tempfile
...
def ...:
...
now_str = datetime.now().strftime(...)
prefix = f"{self.label}-{getpass.getuser()}-{now_str}-"
tmp_dir = tempfile.TemporaryDirectory(prefix=prefix, delete=False) At which point I'm not saying it's the best solution in this precise scenario, but I do observe that temporary files are not as immediately trivial as many assume at first glance — there's a reasion |
I like that option. I was also hoping to delete the directory at the end and it takes care of that. Thanks! |
I just merged this into my kubernetes test PR #3482 after getting distracted making task vine work in there, to give me some more assurance about how this works when workers do not have a shared filesystem with the submit side. It works ok. |
This moves the working directory for TaskVine executor function data to a directory in /tmp These files were previously written adjacent to the logging in the working directory. They may be written in excess when running a large number of tasks.
Description
This moves the working directory for TaskVine executor function data to a directory in /tmp
These files were previously written adjacent to the logging in the working directory. They may be written in excess when running a large number of tasks.
Type of change
Choose which options apply, and delete the ones which do not apply.