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

docs: add pytket-docs-theming submodule #392

Merged
merged 22 commits into from
Oct 2, 2024
Merged

docs: add pytket-docs-theming submodule #392

merged 22 commits into from
Oct 2, 2024

Conversation

CalMacCQ
Copy link
Contributor

@CalMacCQ CalMacCQ commented Sep 17, 2024

Description

Essentially a clone of CQCL/pytket-cutensornet#153 but for pytket-qiskit.

Using the pytket-docs-theming repo as a source of truth for the different extension modules.

Also took the oppurtunity to clean up the legacy docs build.

Added scripts for installing the docs dependencies and building the docs (b0b55c9). These scripts will not be used in the website build... Only in this repository.

My local build below

Screenshot 2024-09-17 at 18 08 21

I note that theres a decent amount of duplication in the github workflows as the docs are built multiple times (see docs.yml and build_and_test.yml. Should we update these at this point? Or should that be done at a later time?

Related issues

CQCL/pytket-docs#360

Checklist

  • I have performed a self-review of my code.
  • I have commented hard-to-understand parts of my code.
  • I have made corresponding changes to the public API documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the changelog with any user-facing changes.

@CalMacCQ CalMacCQ requested a review from cqc-melf as a code owner September 17, 2024 13:19
@CalMacCQ CalMacCQ marked this pull request as draft September 17, 2024 13:30
@@ -80,6 +80,7 @@ jobs:
if: (matrix.os == 'ubuntu-22.04') && (github.event_name == 'pull_request' || github.event_name == 'schedule' )
run: |
cd docs && bash ./install.sh
poetry run pip install ../.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is how I should be installing the pytket-qiskit version in this workflow.

Would it be better to install a .whl directly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there should be a wheel available to install at this point.
Could you try something like below?

run: for w in `find wheelhouse/ -type f -name "*.whl"` ; do poetry install $w ; done

Or if that is not working

run: for w in `find wheelhouse/ -type f -name "*.whl"` ; do poetry run pip install $w ; done

@CalMacCQ CalMacCQ marked this pull request as ready for review September 17, 2024 16:21
@CalMacCQ CalMacCQ changed the title docs: add new docs theming docs: add pytket-docs-theming submodule Sep 17, 2024
@@ -128,14 +132,16 @@ jobs:
path: wheelhouse
- name: Install pip, wheel
run: pip install -U pip wheel
- name: Install poetry
run: pip install poetry
- name: Install extension
run: for w in `find wheelhouse/ -type f -name "*.whl"` ; do pip install $w ; done
Copy link
Contributor Author

@CalMacCQ CalMacCQ Sep 17, 2024

Choose a reason for hiding this comment

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

I'm not sure what do do with this line as I don't think the wheel is installed in the poetry env

Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, I would replace this with:

Copy link
Collaborator

@cqc-melf cqc-melf left a comment

Choose a reason for hiding this comment

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

I have a few questions / suggestions.

I think it might be good to try a release candidate from this branch to check if this works?

@@ -80,6 +80,7 @@ jobs:
if: (matrix.os == 'ubuntu-22.04') && (github.event_name == 'pull_request' || github.event_name == 'schedule' )
run: |
cd docs && bash ./install.sh
poetry run pip install ../.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there should be a wheel available to install at this point.
Could you try something like below?

run: for w in `find wheelhouse/ -type f -name "*.whl"` ; do poetry install $w ; done

Or if that is not working

run: for w in `find wheelhouse/ -type f -name "*.whl"` ; do poetry run pip install $w ; done

@@ -128,14 +132,16 @@ jobs:
path: wheelhouse
- name: Install pip, wheel
run: pip install -U pip wheel
- name: Install poetry
run: pip install poetry
- name: Install extension
run: for w in `find wheelhouse/ -type f -name "*.whl"` ; do pip install $w ; done
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, I would replace this with:

.github/workflows/build_and_test.yml Outdated Show resolved Hide resolved
- name: Test building docs
timeout-minutes: 20
timeout-minutes: 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you reduced the timeout on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, would you prefer I changed it back?

@CalMacCQ CalMacCQ merged commit 01110e6 into main Oct 2, 2024
7 checks passed
@CalMacCQ CalMacCQ deleted the docs/new_theme branch October 24, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants