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

WIP: Check size of input files to handle nfs sync issues #62

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mirkovogel
Copy link

@mirkovogel mirkovogel commented Jul 2, 2021

After a discussion with @critias , we opted for the following design, handling sync issues both between jobs and between tasks.

  • When a task finishes, the LoggingThread writes a the size and the mtime of all files below work and output to the ressources file (-> Job._sis_get_file_stats)
  • When a task is set up, it calls Task._wait_for_input_to_sync. There the expected sizes are obtained by calling Job._sis_get_expected_file_sizes(job_dir, task) ...
    • for all jobs it depends upon, if it is the first task (then only the files lists as inputs are retained)
    • for the preceding task otherwise.
  • The expected file sizes are then compared to the actual sizes.

There are two new config keys:

  • WAIT_PERIOD_CHECK_FILE_SIZE
  • MAX_WAIT_FILE_SYNC

@curufinwe
Copy link
Collaborator

Note that the finished file is also put in the finished.tar.gz file at job cleanup.

@mirkovogel mirkovogel force-pushed the mvogel-check-output-size branch from 494f778 to 0f2e67b Compare July 5, 2021 10:54
Copy link
Contributor

@critias critias left a comment

Choose a reason for hiding this comment

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

The overall approach looks good to me, let's see if it works as expected.

sisyphus/job.py Show resolved Hide resolved
sisyphus/task.py Outdated Show resolved Hide resolved
@mirkovogel
Copy link
Author

Side note: This change does not break old setups. If no size info is available for a given job / task, no checks are run.

@@ -210,6 +210,10 @@ def file_caching(path):
WAIT_PERIOD_JOB_CLEANUP = 10
#: How many seconds should all inputs be available before starting a job to avoid file system synchronization problems
WAIT_PERIOD_MTIME_OF_INPUTS = 60
Copy link
Contributor

Choose a reason for hiding this comment

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

WAIT_PERIOD_MTIME_OF_INPUTS can be removed since the check inside of task is removed. Setting it should also be removed in toolkit.setup_script_mode

except AttributeError:
job_dir = job

if os.path.exists(os.path.join(job_dir, gs.JOB_FINISHED_ARCHIVE)):
Copy link
Contributor

@critias critias Jul 8, 2021

Choose a reason for hiding this comment

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

This could alternatively also be checked doing something like this:

import tarfile
tf=tarfile(os.path.join(job_dir, gs.JOB_FINISHED_ARCHIVE))
for n in tf:
  if n.path.startswith('usage.'):
    usage = literal_eval(tf.extractfile(n.path).read())

but I'm not sure if this would be worth the effort. The given solution should work in nearly all cases if the clean up timeout is large enough.

# If the job has been cleaned up, no size info is available, but we can safely
# assume that enough time has passed so that all files are synced.
if other_job_sizes:
expected_sizes[i.rel_path()] = other_job_sizes[i.rel_path()]
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails if a path is only used as prefix without representing a real file. We could change it to something like this:

try:
    expected_sizes[rel_path] = other_job_sizes[rel_path]
except KeyError:
    for k, v in other_job_sizes.items():
        if k.startswith(rel_path):
            expected_sizes[k] = v

This could also be used if the path is pointing to a directory.

if time.time() - start > timeout:
logging.error("%s not synced for more than %ds, file_stats still empty.", fn, timeout)
raise TimeoutError
logging.info("%s not synced yet, file_stats still empty.", fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause problems if a path is available and should be used before the job is finished e.g. training of a neural model.
We could require that these paths have a special attribute set. They could then be excluded from this check.

while True:
with open(fn) as f:
try:
stats = literal_eval(f.read())["file_stats"]
Copy link
Contributor

Choose a reason for hiding this comment

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

It can happen that this raises a SyntaxError if the file is accessed while it's being written.

logging.info("Inputs:\n%s", "\n".join( str(i) for i in self._job._sis_inputs))

try:
self._wait_for_input_to_sync()
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be could to be able to switch off the new check with an entry in the setting file if something goes wrong. Even better: switch back to the old timeout.

try:
self._wait_for_input_to_sync()
except TimeoutError:
self.error(task_id, True)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really stop the task from running. It's sets the error marker and then continues. Once the task is finished a finished marker is set and the error marker is ignored.

@mirkovogel
Copy link
Author

@critias : I wanted to get this PR merged before "vacation" (=no kindergarden) starts, which didn't happen because I spent last week in bed, not in front of a computer screen. As I won't be able to do so until 8/13, I invite you to take over this PR. :-)

@mirkovogel
Copy link
Author

@critias I got sidetracked for quite some time ... How about the current situation of the cluster? Is it running so smoothly that this PR has become obsolete? Starting next week I'd have time to implement the changes you suggested.

@critias
Copy link
Contributor

critias commented Sep 14, 2021

I continued working on it here: https://github.com/rwth-i6/sisyphus/tree/check-output-size
It seems to be working ok for me, but I got also sidetracked since the overall situation got better. Let's have a call discuss how to continue from here.

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