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

Add job_tags and max_execution_time to options #510

Closed
wants to merge 17 commits into from

Conversation

kt474
Copy link
Member

@kt474 kt474 commented Aug 30, 2022

Summary

Details and comments

Fixes #473

@coveralls
Copy link

coveralls commented Aug 30, 2022

Pull Request Test Coverage Report for Build 3191875458

  • 15 of 19 (78.95%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 65.369%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/ibm_backend.py 0 4 0.0%
Totals Coverage Status
Change from base Build 3190346114: 0.3%
Covered Lines: 3392
Relevant Lines: 5189

💛 - Coveralls

qiskit_ibm_runtime/options.py Outdated Show resolved Hide resolved
Comment on lines 289 to 290
"job_tags": options.get("job_tags"),
"max_execution_time": options.get("max_execution_time"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is actually meant to return only fields inside the RuntimeOptions class to be passed to QiskitRuntimeService.run(). The docstring of QiskitRuntimeService.run() says the options parameter can only be backend, image, and log_level. If you want to change that you'll need to update docstring + release note.

Comment on lines 107 to 108
max_execution_time: Maximum execution time in seconds. This overrides
the max_execution_time of the program and cannot exceed it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this description a bit confusing. What does "overrides the max_execution_time of the program" mean? Does it update the max_execution_time metadata of a program? I don't think that's how this works?

the max_execution_time of the program and cannot exceed it.

job_tags: Tags to be assigned to the job. The tags can subsequently be used
as a filter in the :meth:`jobs()` function call.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
as a filter in the :meth:`jobs()` function call.
as a filter in the :meth:`qiskit_ibm_runtime.qiskit_runtime_service.jobs()` function call.

There is no jobs() method in this file so the reference link needs to be fully qualified.

@@ -860,6 +860,7 @@ def run(
inputs: Program input parameters. These input values are passed
to the runtime program.
options: Runtime options that control the execution environment.
``job_tags`` and ``max_execution_time`` can also be set here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think your intent is to move job_tags and max_execution_time to be part of RuntimeOptions class. If that's the case, then 1) the use of them as separate input args should be deprecated and 2) docstring should list them as bullet points, similar to backend, image, and log_level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this something we want to do? Deprecate job_tags and max_execution_time as input args and have them passed in through in options instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah since I think it's rather confusing to have some options inside options and some outside.

@@ -860,6 +860,7 @@ def run(
inputs: Program input parameters. These input values are passed
to the runtime program.
options: Runtime options that control the execution environment.
``job_tags`` and ``max_execution_time`` can also be set here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah since I think it's rather confusing to have some options inside options and some outside.

@@ -191,6 +198,8 @@ class Options:
Default: ``True``.
"""

max_execution_time: int = None
job_tags: List[str] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

I created a new EnvironmentOptions in my last PR as an attempt to organize less commonly used options (such as image) that is not related to transpilation or execution. I think job_tags can go there too, but max_execution_time can stay at level 1 as it's more likely to be used.

@kt474
Copy link
Member Author

kt474 commented Oct 8, 2022

closing in favor of #565

@kt474 kt474 closed this Oct 8, 2022
@kt474 kt474 deleted the add-options branch July 13, 2023 21:49
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.

Options should include job tags and max time
3 participants