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 params to nb-tester #2350

Merged
merged 23 commits into from
Nov 21, 2024
Merged

Add params to nb-tester #2350

merged 23 commits into from
Nov 21, 2024

Conversation

emilkovacev
Copy link
Collaborator

Migrating from #2344

Effectively allows the nb-tester script to be run against any backend. Also allows you to pass authentication and configuration directly to QiskitRuntimeService (if preferred).

Here's an example of how you can use the new changes to run a set of notebooks against test_eagle using qiskit_ibm_runtime:

test-docs-notebooks --config-path scripts/config/notebook-testing.toml --provider "qiskit_ibm_runtime" --backend "test_eagle2"

By default, --provider is set to qiskit_fake_provider, which runs the same provider code that was present in nb_tester before. By changing the provider, you can also specify "runtime_fake_provider" to use the fake_providers in qiskit_ibm_runtime.fake_provider, or "qiskit_ibm_runtime" to rely on real backends, and the credentials provided.

This shouldn't require changes in CI yet - it seems like notebook testing CI is able to complete the same number of tests as in main, although it does carry over some errors that I ran into on main too.

@qiskit-bot
Copy link
Contributor

Thanks for contributing to Qiskit documentation!

Before your PR can be merged, it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. Thanks! 🙌

Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This looks great! Please wait for @frankharkins to approve before merging

scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@frankharkins frankharkins left a comment

Choose a reason for hiding this comment

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

Looks great! Just one last thing

scripts/nb-tester/qiskit_docs_notebook_tester/__init__.py Outdated Show resolved Hide resolved
if not runtime_service_args:
runtime_service_args = {}
qiskit_runtime_service_args = ", ".join(
f"{arg}=\"{val}\"" if isinstance(val, str) else f"{arg}={val}" for arg, val in generic_backend_args.items()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bug that you're using generic_backend_args rather than runtime_service_args. Generally, I recommend deduplicating this code and simplifying it by having the caller pass in the args, so the caller is responsible for doing things like adding "". That will be easier for them to do because they know the type of each argument. This means that this function would take in runtime_service_args = list[str] | None = None (the None is only because [] is not a safe default arg in Python)

Then the caller would do something like:

runtime_service_args = [
                f'channel="{config.args.channel}"'            ],

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only issue with this example (which I ran into while trying this out) is if the args aren't given, then config.args.channel is "None", which is invalid - which either means we write the following for each option:

f'channel="{config.args.channel}"' if config.args.channel else "channel=None"

Or, I could instead make that initial long join statement into a generic function since we would use it multiple times (and are likely to make use of it again):

def render_kwarg(arg, val):
    if isinstance(val, str):
        return f"{arg}=\"{val}\""
    return f"{arg}={val}"

and if there are any more use cases we can expand the usage of the function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

render_kwarg

Sounds good to me.

Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Looks great! Feel free to merge after those last suggestions. Thanks for iterating on this!

@emilkovacev emilkovacev added this pull request to the merge queue Nov 21, 2024
Merged via the queue into main with commit bc103b4 Nov 21, 2024
3 checks passed
@emilkovacev emilkovacev deleted the ek-nb-test branch November 21, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants