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

Job creation is being done in two steps #1150

Closed
Tansito opened this issue Dec 15, 2023 · 9 comments · Fixed by #1217
Closed

Job creation is being done in two steps #1150

Tansito opened this issue Dec 15, 2023 · 9 comments · Fixed by #1217
Assignees
Labels
enhancement New feature or request priority: medium Medium priority project: gateway Label to identify features related with gateway project
Milestone

Comments

@Tansito
Copy link
Member

Tansito commented Dec 15, 2023

What is the expected enhancement?

From #1147 @akihikokuroda suggested that the current logic that creates a Job can be problematic due to that the scheduler may find the job between the current 2 save() and execute it without the environment information.

Two options that come to my mind are:

  • Have a new state before QUEUED that reflects some kind of an under state preparation.
  • Remove job.id from environment variables but I think we need to have it

Happy to hear your thoughts @akihikokuroda , @IceKhan13 , @psschwei 😄

@Tansito Tansito added project: gateway Label to identify features related with gateway project enhancement New feature or request priority: medium Medium priority labels Dec 15, 2023
@Tansito Tansito added this to the Q1'24 milestone Dec 15, 2023
@psschwei
Copy link
Collaborator

Adding a new state might be an issue... didn't someone want our states to map directly to the qiskit / runtime ones?

Assuming it is, could we add logic in the scheduler to say don't execute if jobid is null?

@Tansito
Copy link
Member Author

Tansito commented Dec 15, 2023

Well this new state wouldn't need even a migration for old Jobs and so on and the mapping for the states was a suggestion from Caleb for those states that represents the same in qiskit / runtime. I would not be worried about it specifically. That being said, check by job.id is another solution. Pros and Cons that I see:

  • cons:
    • To run a Job then we will need:
      • Check the status
      • Decrypt the environment variables
      • Check the value for the job.id
    • Logic a bit more confusing than just check a status in case we want to debug
  • pros:
    • easier to implement, less places where we need to touch
    • status remains the same so we don't have new ones in our statistics to take them into account

From my point of view. I would go with @psschwei proposal by now with the condition that if we start to add exceptions to when we should run a Job then we should re-think the implementation. That has it sense? (Other proposals / opinions continue being welcome).

@psschwei
Copy link
Collaborator

For job status, the issue I was thinking of was #1039

@akihikokuroda akihikokuroda self-assigned this Feb 9, 2024
@akihikokuroda
Copy link
Collaborator

I'm wondering why this save() is necessary.

@Tansito
Copy link
Member Author

Tansito commented Feb 9, 2024

Good question @akihikokuroda . In my refactorization I found that we were using the job_id obtained from that save in the build_env_variables method in this next line. From what I know we were using it for traceability but maybe we could review it.

@akihikokuroda
Copy link
Collaborator

The Job.id is filled when the Job instance is created. The build_env_variables takes the Job.id from the job instance passed it. So the Job instance doesn't need to be saved to get the Job.id.

@Tansito
Copy link
Member Author

Tansito commented Feb 9, 2024

Exactly, and the job instance is created in that point.

@Tansito
Copy link
Member Author

Tansito commented Feb 9, 2024

This is exactly what I was trying to solve with the services @akihikokuroda . If there is another part of the code where we are creating jobs then we need to refactorize it to use the new service. Don't remove the service.

@Tansito
Copy link
Member Author

Tansito commented Feb 12, 2024

Thank you @akihikokuroda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: medium Medium priority project: gateway Label to identify features related with gateway project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants