-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Don't refresh the job as soon as we receive the result. #7662
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
Conversation
- Currently, each streaming result refreshes the result. - Forgoing this can reduce batch result times by 3x.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7662 +/- ##
==========================================
- Coverage 99.37% 99.37% -0.01%
==========================================
Files 1082 1082
Lines 96700 96689 -11
==========================================
- Hits 96097 96085 -12
- Misses 603 604 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
def __init__( | ||
self, | ||
*, # Forces keyword args. | ||
job_id: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resource seems to map to a QuantumResult
API resource, but the class doesn't contain the unique identifier for that resource - job_id
is only unique within a QuantumProgram parent
, which is only unique within a GCP project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's a pre-existing issue, but I could add more fields while I am here. What else should we add, if anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project_id
and program_id
should be included as well and match the values in the parent EngineJob
. Feel free to leave as a followup or file a bug since it's out of scope of this change if you aren't going to lazy load the timestamp of the job's completion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will do that in a follow-up.
*, # Forces keyword args. | ||
job_id: str, | ||
job_finished_time: datetime.datetime, | ||
job_finished_time: datetime.datetime | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere besides tests and places working around the requirement to set the field? Could we just remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally removed.
def __init__( | ||
self, | ||
*, # Forces keyword args. | ||
job_id: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project_id
and program_id
should be included as well and match the values in the parent EngineJob
. Feel free to leave as a followup or file a bug since it's out of scope of this change if you aren't going to lazy load the timestamp of the job's completion.
Uh oh!
There was an error while loading. Please reload this page.