-
Notifications
You must be signed in to change notification settings - Fork 80
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 tests for code in serverless-manage-resources.ipynb
#1960
base: main
Are you sure you want to change the base?
Conversation
The jobs do not use QPU time and are fast enough to complete in the timeout window
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
One or more of the following people are relevant to this code: |
Unlikely, but we could have a problem if CI runs two versions of this notebook at the same time. Setting a random ID solves that
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.
Thanks for working on this!
@@ -22,22 +22,53 @@ | |||
}, |
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 think line 20 should be transpile_remote.py
. /guides/serverless-first-program#get-program-arguments also uses the wrong file name
" title=TITLE,\n", | ||
" entrypoint=\"transpile_remote.py\",\n", | ||
" working_dir=\"./source_files/\",\n", | ||
"\tdependencies=[\"qiskit-aer==0.14.1\"],\n", |
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.
Why is this hardcoded? I'm not very familiar with serverless
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 think it's just to demonstrate that you can set dependencies for your serverless code
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.
Hm, I wonder if we can remove it. I'm worried about the dep falling out of sync. Think about how many people copy and paste code, or even Qiskit Code Assistant learning from our docs. We don't want them installing an old version.
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.
lol I'm sorry; I didn't realise this was part of the testing code I added. I copied it from https://docs.quantum.ibm.com/guides/serverless-first-program. I'll remove the dep.
Think about how many people copy and paste code
👀
"source": [ | ||
"```python\n", | ||
"# source_files/transpile_parallel.py\n", | ||
"%%writefile ./source_files/transpile_parallel.py\n", |
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.
Do you know why we change file names here?
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.
Not really, I guess they could both be transpile_remote
EDIT: Actually I think it's helpful to make it clear they're different programs
"from qiskit.circuit.random import random_circuit\n", | ||
"from qiskit_serverless import IBMServerlessClient, QiskitFunction\n", | ||
"import time\n", |
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'd probably be good to DRY this code. Can you define a hidden helper function at the top that gets called by both?
324c44e
to
9bce5ad
Compare
Co-authored-by: Eric Arellano <[email protected]>
…H/serverless-test
Converting to draft as I'm struggling to work around a limitation. The problem is that |
…rces" (#2015) This makes the code examples runnable. Unfortunately we can't add tests just yet (will happen in #1960), but this does mean users will be able to run all the code on the page. I've removed the section on automatic QPU selection after discussion with the serverless team. This feature is being developed but isn't ready just yet. We'll add the section back in when it's ready.
This PR fixes the code in
serverless-manage-resources.ipynb
and adds tests to check it runs correctly.Link to preview: https://qiskit.github.io/documentation/pr-1960/guides/serverless-manage-resources