Skip to content

Make dev_tools/nbfmt more careful about tensorflow-docs #219

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

Merged
merged 10 commits into from
Mar 10, 2025
23 changes: 17 additions & 6 deletions dev_tools/nbfmt
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,34 @@
cd "$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" || exit
cd "$(git rev-parse --show-toplevel)" || exit

# 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we planning to download from cirq from other repos too?

Copy link
Contributor Author

@mhucka mhucka Mar 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is still downloading from https://raw.githubusercontent.com/quantumlib/Cirq/main I am not sure I understand your comment.

Copy link
Contributor Author

@mhucka mhucka Mar 9, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mhucka mhucka Mar 10, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


# Check if cirq/check/nbformat exists; if not, grab it.
if [[ ! -f dev_tools/nbformat ]]; then
wget https://raw.githubusercontent.com/quantumlib/Cirq/master/check/nbformat -P dev_tools/
wget $cirq/check/nbformat -P dev_tools/
chmod u+x dev_tools/nbformat
fi

# Check if tensorflow-docs.txt requirements file exists, if not grab it.
# Check if the tensorflow-docs.txt requirements file exists; if not, grab it.
if [[ ! -f dev_tools/tensorflow-docs.txt ]]; then
wget https://raw.githubusercontent.com/quantumlib/Cirq/master/dev_tools/requirements/deps/tensorflow-docs.txt -P dev_tools/
wget $cirq/dev_tools/requirements/deps/tensorflow-docs.txt -P dev_tools/
fi

# Check if the tensorflow-docs package has been installed; if not, install it.
if ! pip show --quiet tensorflow-docs > /dev/null 2>&1; then
pip install -r dev_tools/tensorflow-docs.txt
fi

# Run the formatter.
result=$(dev_tools/nbformat "$@")
status=$?

# Make sure error message references right file.
result=${result//"check/nbformat"/"dev_tools/nbfmt"}
# If there is an error message, make sure it references this script.
# Note: using vars here avoids a problem with subsituting strings containing /.
check_path="check/"
devtools_path="dev_tools/"
result=${result//$check_path/$devtools_path}
result=${result//"nbformat"/"nbfmt"}
printf '%s\n' "${result[@]}"
exit $status