Make dev_tools/nbfmt more careful about tensorflow-docs#219
Make dev_tools/nbfmt more careful about tensorflow-docs#219mhucka merged 10 commits intoquantumlib:mainfrom
Conversation
If the user previously installed unitary in one virtual environment, did the `pip install -r requirements.txt`, ran `check/nbfmt` (which in turn installs `dev_tools/nbformat` and tensorflow-docs), then at a later time created a different virtual environment, did the same installation steps, and ran `check/nbfmt`, it would fail because the presence of `dev_tools/nbformat` fooled `nbfmt` into thinking tensorflow-docs was already installed. A solution is to decouple of check for the nbformat script and the tensorflow-docs package. I also tweaked the error-message rewriting code at the very end, because it wasn't doing the substitution in my environment. (That might be because the stock version of Bash on MacOS is an old 3.2.)
|
Note: the current CI check failures should disappear after PR #220 is applied. |
|
|
||
| # Check if cirq/check/nbformat exists, if not grab it. | ||
| # Base URL for downloading tools from Cirq. | ||
| declare -r cirq=https://raw.githubusercontent.com/quantumlib/Cirq/main |
There was a problem hiding this comment.
Are we planning to download from cirq from other repos too?
There was a problem hiding this comment.
Are we planning to download from cirq from other repos too?
Ah, no; the reason I assigned a variable is that the same URL is used in two places (as it was in the previous version of this script too). It seems better to keep things DRY and write it once.
There was a problem hiding this comment.
It is still downloading from https://raw.githubusercontent.com/quantumlib/Cirq/main I am not sure I understand your comment.
There was a problem hiding this comment.
Ok, maybe I don't understand your question :-).
You had asked "Are we planning to download from cirq from other repos too?", which I interpreted as asking whether we'd download cirq from repos other than the cirq repo. The answer to that is no – I can't think of a reason to download it from a repo other than cirq's repo. I was puzzled by why you were asking, so I thought maybe the reason you were asking is that it's unclear why the download location is being assigned to a variable when that value will never change. The reason for that is that the location is used in more than one place in the script.
Could you clarify what "download from cirq from other repos" means?
There was a problem hiding this comment.
This PR is for the unitary repo but the url is downloading tools from the cirq repo:
declare -r cirq=https://raw.githubusercontent.com/quantumlib/Cirq/main```
I think we should keep the repos self-contained and not download tools from the cirq repo unless there is a good reason.
There was a problem hiding this comment.
Oh, I understand now.
The reason for this is simply that the original nbfmt in unitary does this. (This can be seen in the version in main currently.) I didn't want to question the original choice so left that part the same, and only cleaned up the code in other ways.
I could rewrite that, if that would be preferable?
There was a problem hiding this comment.
Oh, I see. I didn't realize that this was preexisting. Yeah, maybe we could make this more independent, but it's lower priority and can be done in a later PR.
mhucka
changed the title
2 participants
If the user previously installed unitary in one virtual environment, did the
pip install -r requirements.txt, rancheck/nbfmt(which in turn installsdev_tools/nbformatand tensorflow-docs), then at a later time created a different virtual environment, did the same installation steps, and rancheck/nbfmt, it would fail because the presence ofdev_tools/nbformatfoolednbfmtinto thinking tensorflow-docs was already installed. A solution is to decouple of check for the nbformat script and the tensorflow-docs package.I also tweaked the error-message rewriting code at the very end, because it wasn't doing the substitution in my environment. (That might be because the stock version of Bash on MacOS is an old 3.2.)