Skip to content

Print job complete percentage in manager (if available) - #302

Open
dorian-K wants to merge 4 commits into
rwth-i6:masterfrom
dorian-K:completed-fraction
Open

Print job complete percentage in manager (if available)#302
dorian-K wants to merge 4 commits into
rwth-i6:masterfrom
dorian-K:completed-fraction

Conversation

@dorian-K

Copy link
Copy Markdown
Contributor
image

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the manager’s running-job status line to show a job’s completion percentage when the job provides a completed_fraction() value, improving at-a-glance progress visibility during execution.

Changes:

  • Extend Manager.print_job_state() to append completed_fraction() as a [xx.x%] suffix for running jobs (when available).
  • Preserve existing behavior of appending job.info() output for running jobs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dorian-K
dorian-K requested a review from albertz June 11, 2026 13:22
Comment thread sisyphus/manager.py Outdated
@dorian-K

Copy link
Copy Markdown
Contributor Author
image Added ETA until job finishes running, with red text when the eta surpasses the job's time rqmt

Comment thread sisyphus/manager.py Outdated
Comment thread sisyphus/manager.py Outdated
Comment thread sisyphus/manager.py Outdated
@michelwi

Copy link
Copy Markdown
Contributor

I want to raise the question "should we really do this?"

  • I only took a coursory glance, but it seems to me that for each running job we need to open and parse 2 or 3 files. I already foresee problems for people that have 1k+ running jobs on a slow filesystem day.
  • Also this update is printed quite often (when I have so many running jobs, i tend to get an update every other minute)
  • Not sure how resilient your implemented logic is, but there seem to be a lot of safeguards necessary (if a: [program continues] or try: ... Except) Are we sure this does not break easily?
  • The implementation has huge levels of indentation and is not easy to read. I would suggest to transform many parts of the form
if a:
   [expected case, long code follows]

into

if not a:
    [Handle problem; e.g. pass, continue, or set a default]
[continue normal execution]

which improves readability. Maybe even put some code in a separate function and then just return "" in case of problems.

And all of this just for the tiny additional information about some estimated finish time (which I think is irrelevant as I anyway cannot change it and the job finishes when it does 🤷 )

@albertz

albertz commented Jun 11, 2026

Copy link
Copy Markdown
Member

I only took a coursory glance, but it seems to me that for each running job we need to open and parse 2 or 3 files.

No. Not more files than before are being opened. This is only for jobs which support the completed_fraction, which is mostly just the ReturnnTrainingJob currently. And already right now it opens those files to print the progress in what epoch it currently is. This here basically just extends that, and works in general on any other job that supports the completed_fraction.

You are right that the LR file that is opened by ReturnnTrainingJob is now opened twice right after each other due to first calling info and then completed_fraction. We could maybe cache this, but the OS level cache might also handle that well already.

So I'm not sure this is really such an issue.

Comment thread sisyphus/manager.py Outdated
Comment thread sisyphus/manager.py Outdated
@dorian-K

Copy link
Copy Markdown
Contributor Author

I've read through your comments and i agree with most of them. I actually already noticed a slight slowdown with only 30 running jobs, as before the print was instant, and now it printed line-by-line for maybe 100-200ms until it was done. So for thousands of jobs this may get super annoying.

This and the slurm-specific issue could likely be improved by using the file's metadata ("birthtime"), but this is, at the moment, not easily accessible in python without external libraries or subprocess calls. And anyways it would still be a crude estimate and be wildly inaccurate for interrupted/rescheduled jobs as you've said above.

So i have decided to remove the ETA thing again and only keep the percentage. Probably better to implement this feature individually for some (long running) jobs in their info() to get more accurate numbers

@albertz

albertz commented Jun 12, 2026

Copy link
Copy Markdown
Member

file's metadata ("birthtime")

Python 3.12+ populates this on compatible Linux systems os.stat(path).st_birthtime?

Or Python 3.11 and Older:

info = os.statx(os.AF_DECNET, path, mask=os.STATX_BIRTHTIME)
    
# Check if the filesystem actually returned the birthtime
if info.stx_mask & os.STATX_BIRTHTIME:
    birthtime = info.stx_btime.tv_sec

@dorian-K

Copy link
Copy Markdown
Contributor Author
❯ ./.venv/bin/python
Python 3.12.13 (main, Mar 20 2026, 00:33:26) [Clang 22.1.1 ] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> a = os.stat("work/i6_experiments/users/dorian_koch/speech_llm/moshi/MoshiAnnotate.Y6urTHFSgFbE.cleared.0001/log.run.1")
>>> a.st_birthtime
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'os.stat_result' object has no attribute 'st_birthtime'
>>> a
os.stat_result(st_mode=33184, st_ino=324265055682040913, st_dev=1990885514, st_nlink=2, st_uid=46724, st_gid=50526, st_size=2916, st_atime=1781262983, st_mtime=1781240318, st_ctime=1781240318)
>>> os.statx
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'os' has no attribute 'statx'. Did you mean: 'stat'?

@dorian-K

Copy link
Copy Markdown
Contributor Author

The command stat does give the birthtime though, so not an os issue

@albertz

albertz commented Jun 13, 2026

Copy link
Copy Markdown
Member

Ok I guess then we would need a better way to get the job start time. Maybe extending the engine API or so. Or storing it explicitly in a file when the job starts.

@dorian-K

Copy link
Copy Markdown
Contributor Author

Ok I guess then we would need a better way to get the job start time. Maybe extending the engine API or so. Or storing it explicitly in a file when the job starts.

Yes. But in the case of interrupted jobs that need multiple submissions to finish (i.e. training jobs), this would again be inaccurate, just like the previous approach. Perhaps we should leave it for now

But anyways, I believe this pull request is ready to be merged?

@michelwi michelwi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this pull request is ready to be merged?

yes, we can merge as is and work on a better implementation for the ETA later.

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.

4 participants