-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Separate docs build workflow #2441
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2441 +/- ##
=======================================
Coverage 98.70% 98.70%
=======================================
Files 88 88
Lines 4083 4083
=======================================
Hits 4030 4030
Misses 53 53 ☔ View full report in Codecov by Sentry. |
Pinging @natestemen because I want to rerun the jobs in #2447 without waiting for the docs build. Once this PR is merged, I could rebase the other branch. |
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 don't see any problems with this. We'll see how it goes, and can revert if any problems arise.
.github/workflows/docs-build.yml
Outdated
- name: Install Python dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
make install requirements |
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.
IIRC this should do the same thing. Can you double check? Maybe in the past it was a different command, but now I think make install xyz
performs the same action as make install
.
make install requirements | |
make install |
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 do not know how a makefile works that well. It's on my list of todos.
Maybe in the past it was a different command,
Looking through the targets, why were we using make install requirements
in the past? According to this, make install
will install from the first installation target which is install
. I see development
is utilized in setup.py
where we install things listed in requirements/
.
Lines 49 to 51 in 231319d
.PHONY: install | |
install: | |
pip install -e .[development] |
Lines 59 to 61 in 231319d
.PHONY: requirements | |
requirements: requirements/requirements.txt | |
pip install -r requirements/requirements.txt |
Why do we have two separate installation targets in the makefile? IMO both appear to behave the same unless I am missing something.
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.
pip install -e .[development]
This will install
- an editable installation of Mitiq (
-e
is short for--editable
) - all the dependencies (the
[development]
"tag" is defined insetup.py
here to mean all of the dependencies found inrequirements/
.)
pip install -r requirements/requirements.txt
This will only install the requirements listed in requirements/requirements.txt
which is currently numpy
, scipy
, cirq-core
, and tabulate
. I can't find any uses of this command in Mitiq and it should be safe to remove. I've added a note for the community call this week to discuss.
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.
Discussed in the community call. Green light from everyone on removing it.
@purva-thakre is going to create a PR to remove it (no need for a ticket, we can just refer to this discussion) and there we will make sure all the builds pass.
Co-authored-by: nate stemen <[email protected]>
Going to go ahead and merge this if |
Description
As discussed during the community call today, this PR splits the docs build job into a separate workflow. This makes it easier for us to rerun pytest related jobs without waiting for the docs build to be completed.
After this PR is merged, #2437 will be rebased.
License
Before opening the PR, please ensure you have completed the following where appropriate.