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

feat: add execution duration as a information field in Job and Submission entities #1640

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

trgiangdo
Copy link
Member

Resolves #1544

Now, in Job and Submission entities, there are new information fields:

  • execution_started_at
  • execution_ended_at
  • execution_duration

In the Job entity, only the execution_started_at and execution_ended_at are stored in the model. execution_duration is calculated from the other 2.

In the Submission entity, execution_started_at, execution_ended_at, and execution_duration are calculated from the Submission.jobs.

@FlorianJacta
Copy link
Member

It may be unnecessary to have this inside the job execution_duration or not. In my mind, I want to have this information inside the job selector. Up to you!

@FabienLelaquais
Copy link
Member

It may be unnecessary to have this inside the job execution_duration or not. In my mind, I want to have this information inside the job selector. Up to you!

I want to see use cases before we decide anything.
I don't think this duration should be stored anywhere.
If you "want it in the job selector", the questions will be how, what format, what for, and what if the job is still running. Things like that.

@@ -44,5 +45,7 @@ def _dispatch(self, job: Job):
Parameters:
job (Job^): The job to submit on an executor with an available worker.
"""
job.execution_started_at = datetime.datetime.now()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about creating a context manager to set execution_started_at and execution_end_at? Something like:

import datetime
from contextlib import contextmanager

@contextmanager
def track_execution(job):
    job.execution_started_at = datetime.datetime.now()
    try:
        yield
    finally:
        job.execution_ended_at = datetime.datetime.now()

And them:

  def _dispatch(self, job: Job):
   """Dispatches the given `Job` on an available worker for execution.

   Parameters:
       job (Job): The job to submit on an executor with an available worker.
   """
   with track_execution(job):
       rs = _TaskFunctionWrapper(job.id, job.task).execute()
       self._update_job_status(job, rs)
 

@joaoandre-avaiga
Copy link
Collaborator

Does Taipy rest need to be updated as well to have execution_started_at and execution_ended_at?

@trgiangdo
Copy link
Member Author

Does Taipy rest need to be updated as well to have execution_started_at and execution_ended_at?

@joaoandre-avaiga You're right, REST needs to be updated as well, after we decide which fields to be added.

@trgiangdo
Copy link
Member Author

trgiangdo commented Aug 7, 2024

@FabienLelaquais @FlorianJacta
For now, the use-case is:

  • If the job is not started, both the start and end time will be None.
  • If the job is started and not finished, the end time will be None, and also the duration will be None.
  • If the job is finished, the duration is calculated and returned in seconds.

toan-quach
toan-quach previously approved these changes Aug 7, 2024
Copy link
Member

@toan-quach toan-quach left a comment

Choose a reason for hiding this comment

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

lgtm

FlorianJacta
FlorianJacta previously approved these changes Aug 8, 2024
Copy link
Member

@FlorianJacta FlorianJacta left a comment

Choose a reason for hiding this comment

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

I created a new issue to encapsulate my need of having the job duration inside the job selector: #1646

Otherwise, this looks good to me.

@trgiangdo trgiangdo merged commit a46d9fa into develop Aug 8, 2024
52 of 68 checks passed
@trgiangdo trgiangdo deleted the feature/#1544-add-job-execution-duration branch August 8, 2024 10:04
Copy link
Member

@jrobinAV jrobinAV left a comment

Choose a reason for hiding this comment

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

I would try to be more generic and keep all the timestamps for all the job status changes: SUBMITTED, BLOCKED, PENDING, RUNNING, CANCELED, FAILED, COMPLETED, SKIPPED, ABANDONED.
We should not face any performance impact since a status change already triggers the job update.

With that, we got all the information for the user to compute everything,
like the total execution time, of course, but also the pending time, the running time, the blocked time.

We could have something similar for the submission.

@trgiangdo What do you think?

@trgiangdo
Copy link
Member Author

It can be confusing since CANCELED, FAILED, COMPLETED, SKIPPED, ABANDONED are all counted as is_finished.

However, I believe we can do it.

For submission entity, we can calculate all information from the job list.

@jrobinAV
Copy link
Member

It can be confusing since CANCELED, FAILED, COMPLETED, SKIPPED, ABANDONED are all counted as is_finished.

However, I believe we can do it.

For submission entity, we can calculate all information from the job list.

Storing the information is one thing, but exposing it is different. We can keep all the job status changes timestamps, and we can expose good properties at the job level that compile the information properly.
So we can use it like that:

jobs._records == 
    {
        "SUBMITTED": 2022-09-27 18:00:00.000, 
        "RUNNING": 2022-09-27 18:00:01.000, ..., 
        "COMPLETED": 2022-09-27 18:00:02.000}
jobs.execution_time == 2

We could add some dynamic properties like the following

@property
def execution_time():
    if not "SUBMITTED" in self._records:
        return -1
    if "CANCELED" in self._records:
        return self._records["CANCELED"] - self._records["SUBMITTED"]
    elif "FAILED" in self._records:
        return self._records["FAILED"] - self._records["SUBMITTED"]
    elif "COMPLETED" in self._records:
        return self._records["COMPLETED"] - self._records["SUBMITTED"]
    elif "SKIPPED" in self._records:
        return self._records["SKIPPED"] - self._records["SUBMITTED"]
    elif "ABANDONED" in self._records:
        return self._records["ABANDONED"] - self._records["SUBMITTED"]
    else
        return datetime.now - self._records["SUBMITTED"]

@trgiangdo
Copy link
Member Author

New ticket for improvement has been created: #1704

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add job duration as new information for jobs
6 participants