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

Progress bar in the status message modal + auto-refresh of the modal #2108

Merged
merged 6 commits into from
Aug 25, 2017

Conversation

fvinas
Copy link
Contributor

@fvinas fvinas commented May 4, 2017

Description

Added the ability to display a progress bar in the status message modal using task.set_progress_percentage() + automatically refreshing the modal (for both status message and the progress bar) to follow progress dynamically while it's open.

image

Motivation and Context

  • More visual & practical way to track the progress of running tasks inside Luigi (i.e. not using an external tracking URL). Status message can now be dedicated to text messages instead of progress tracking.

  • Brings dynamic view on a running task when the status message modal is open (elsewhere the UI remains static for now)

Have you tested this? If so, how?

Included unit tests, seems to work well in my environment.

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

This is fantastic. Also the information can (theoretically) be used by so much else (like scheduling ordering).

Looks good to me. @daveFNbuck also mentioned implementing something like this once. :)

class TaskProgressPercentageTest(LuigiTestCase):

def test_run(self):
percentage = 30
Copy link
Contributor

Choose a reason for hiding this comment

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

I think avoiding a variable makes it more readable. :)

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

This all looks cool. Have you used this in production in a while now? Does it work smooth? Can you just go over my comments and then let me know if this is ready to be merged or not.

import luigi.scheduler
import luigi.worker

luigi.notifications.DEBUG = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Tarrasch, yes we're using it in production for a while and it's pretty smooth.
I try to take some time over the week-end to do the changes and I let you know.

task = self._state.get_task(task_id)
return {"taskId": task_id, "progressPercentage": task.progress_percentage}
else:
return {"taskId": task_id, "progressPercentage": None}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait a second. Why don't we return None/{} here or some sort of failure indication, since the task id didn't exist right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes right, I tried to stick to what was already in place in get_task_status_message or fetch_error.
I guess the case may happen if a RPC request is still coming on a given taskId while a task is dead (finished a long time ago but browser still opened in the progress bar display).

In this case luigi/static/visualiser/js/visualiserApp.js should handle it gracefully (hiding the no longer relevant HTML objects).

@dlstadther
Copy link
Collaborator

@fvinas Any update here? Or response to @Tarrasch 's comment/question?

Copy link
Contributor Author

@fvinas fvinas left a comment

Choose a reason for hiding this comment

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

Hi @dlstadther, just checked again and for me everything looks ok?

task = self._state.get_task(task_id)
return {"taskId": task_id, "progressPercentage": task.progress_percentage}
else:
return {"taskId": task_id, "progressPercentage": None}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes right, I tried to stick to what was already in place in get_task_status_message or fetch_error.
I guess the case may happen if a RPC request is still coming on a given taskId while a task is dead (finished a long time ago but browser still opened in the progress bar display).

In this case luigi/static/visualiser/js/visualiserApp.js should handle it gracefully (hiding the no longer relevant HTML objects).

@Tarrasch Tarrasch merged commit 5b80f54 into spotify:master Aug 25, 2017
@Tarrasch
Copy link
Contributor

Thanks @fvinas, I reworded the final commit a bit. I wasn't sure what "modal" means here so I removed it before merging. :)

Anyway expect a release with this patch soon!

This was referenced Jun 29, 2022
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