-
Notifications
You must be signed in to change notification settings - Fork 192
Raise warning if instance limit is reached #2458
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
base: main
Are you sure you want to change the base?
Conversation
|
FYI @francabrera |
ElePT
left a comment
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.
I added one quick question, and I agree that it would be good to get an additional review from @francabrera.
| ) - usage_dict.get("usage_consumed_seconds", 0) | ||
|
|
||
| if limit_reached: | ||
| if not usage_dict.get("usage_limit_seconds") or usage_remaining > 0: |
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.
One question, what if usage_dict.get("usage_limit_seconds") is 0? In this case I believe that not usage_dict.get("usage_limit_seconds") will evaluate to True, but according to the issue we only want it to evaluate to True if the value is None. Did I follow this correctly?
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.
I agree with you on how the 0 case will be handled - depending on how the above conversation evolves, this line might not be present anymore.
| usage_dict = self._active_api_client.cloud_usage() | ||
| limit_reached = usage_dict.get("usage_limit_reached", False) | ||
| usage_remaining = usage_dict.get( | ||
| "usage_limit_seconds", usage_dict.get("usage_allocation_seconds") |
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.
I don't understand why usage_allocation_seconds is used here? If the instance is limited the limit will be usage_limit_seconds.
If it's there is no limit in the instance, but usage_limit_reached is true, then we always display the message for the instance's plan on the account.
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.
It seems related to #1829 - the logic for the existing usage() method also involves usage_allocation_seconds():
qiskit-ibm-runtime/qiskit_ibm_runtime/qiskit_runtime_service.py
Lines 1185 to 1199 in d03882d
| def usage(self) -> Dict[str, Any]: | |
| """Return usage information for the current active instance. | |
| Returns: | |
| Dict with usage details. | |
| """ | |
| usage_dict = self._active_api_client.cloud_usage() | |
| if usage_dict.get("usage_limit_seconds") or usage_dict.get("usage_allocation_seconds"): | |
| usage_remaining = max( | |
| usage_dict.get("usage_limit_seconds", usage_dict.get("usage_allocation_seconds")) | |
| - usage_dict.get("usage_consumed_seconds", 0), | |
| 0, | |
| ) | |
| usage_dict["usage_remaining_seconds"] = usage_remaining | |
| return usage_dict |
@francabrera , can you verify that taking into account usage_allocation_seconds (if usage_limit_seconds) for the calculation is correct? If it is, I think we can just take advantage of the existing method and piggyback on it.
|
Continuing this PR from Kevin (thanks!) - in d8c778b:
|
Summary
Adding warnings before job submission if the instance limit has been reached.
Details and comments
Fixes #2453