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

[on hold] Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes #229

Closed
wants to merge 16 commits into from

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented Mar 27, 2023

Background

We need to differentiate between Qiskit Core/SDK/Terra from the rest of the Ecosystem like Machine Learning. For example, we will have a different logo.

As explained in #216, the simplest way to do this is to distribute multiple themes in our same PyPI package. Users choose which one they want with html_theme in conf.py.

The themes use a "base theme" so that they can reuse as much code as possible. #215 demonstrates how each theme can then override their CSS, HTML templates, and images.

This PR

This PR adds the new themes qiskit_legacy and qiskit_ecosystem_legacy.

For now, these themes are identical to the base theme. This is prework to allow us to start differentiating without needing to worry about e.g. setup.py.

What about the qiskit_sphinx_theme theme?

The original theme qiskit_sphinx_theme will still be distributed, possibly indefinitely. It is set to now use the ecosystem theme, though, since the sole repo that should use qiskit_legacy is Terra.

This means that we will want to switch Terra to explicitly use qiskit_legacy upon the next release of the qiskit_sphinx_theme PyPI package. (Until we start making changes to the ecosystem theme, Terra is fine to keep using qiskit_sphinx_theme; changing its theme right away is more to future-proof.)

Why call it qiskit_legacy?

I also considered qiskit_core, qiskit_terra, and qiskit_sdk. Given that Qiskit Terra will now be called Qiskit, we decided in a docs meeting to simply call it qiskit. There is a slight risk that non-Terra repos use qiskit rather than qiskit_ecosystem, but we think this isn't big enough of a deal and can add a noisy comment in Terra's conf.py to discourage blindly copying and pasting.

CI infrastructure

This builds docs/ with both themes to make sure the build succeeds. In the future, I recommend something like #208 to better automate tests.

It also uploads both builds as artifacts so that we can visually inspect them both in CI.

@Eric-Arellano Eric-Arellano changed the title Add infrastructure for qiskit_sdk__legacy_pytorch and qiskit_ecosystem__legacy_pytorch themes Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes Mar 28, 2023
@Eric-Arellano Eric-Arellano changed the title Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes [WIP] Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes Mar 28, 2023
@Eric-Arellano Eric-Arellano marked this pull request as draft March 28, 2023 15:23
@Eric-Arellano
Copy link
Collaborator Author

Converted to draft because I realized I should update CI to test both themes. Pardon the noise of having asked for reviews before!

@jakelishman
Copy link
Member

I don't mind being asked for review on this package if you think it's something complex you especially wanted me to check out, I'm just making sure that you're not feeling required to request me on every packaging-relating change. It's good to build up the skills within the other members of the docs team, including the possibility of a small error slipping through. I'm happy to help out if there's big stuff that needs checking, though.

@Eric-Arellano
Copy link
Collaborator Author

I don't mind being asked for review on this package if you think it's something complex you especially wanted me to check out, I'm just making sure that you're not feeling required to request me on every packaging-relating change. It's good to build up the skills within the other members of the docs team, including the possibility of a small error slipping through. I'm happy to help out if there's big stuff that needs checking, though.

+1, good point. The original PR indeed did not need your review because you already helped with the setup.py and Manifest.in changes in #216 (thanks!). I'll retune for that.

Now, the CI and tox.ini changes could probably benefit from your look, but not enough to require a review.

@Eric-Arellano Eric-Arellano marked this pull request as ready for review March 30, 2023 15:16
@Eric-Arellano Eric-Arellano changed the title [WIP] Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes Mar 30, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

The packaging-related bits look fine to me 👍

MANIFEST.in Outdated Show resolved Hide resolved
docs/conf.py Outdated
@@ -42,8 +42,7 @@

release = qiskit_sphinx_theme.__version__

html_theme = 'qiskit_sphinx_theme' # use the theme in subdir 'theme'
templates_path = ['_templates']
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to remove templates_path? There does appear to be something in docs/_templates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I meant to comment on this. It was a duplicate of a value lower in the file.

qiskit_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@javabster javabster left a comment

Choose a reason for hiding this comment

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

not a huge deal but would it make more sense to rename the folders to legacy instead of pytorch? Just thinking about new contributors coming to the repo it might not make a lot of sense to see pytorch folders if they don't know the history of the repo

@Eric-Arellano
Copy link
Collaborator Author

not a huge deal but would it make more sense to rename the folders to legacy instead of pytorch?

Or legacy_pytorch? We'll soon also have a furo/ folder, so having legacy/ and furo/ isn't very symmetrical.

The related question is whether to call the subfolder terra/, core/, or I guess we can use just qiskit/?

@javabster
Copy link
Collaborator

javabster commented Mar 30, 2023

ohh I see hmm. I'm just trying to think about what makes the most sense to someone new coming to the repo trying to get started 🤔 how about just legacy and then qiskit/core and qiskit/ecosystem? Then we don't use either pytorch or furo?

And then in future we can just keep moving stuff to that legacy folder and always keep the lastest in the qiskit folder

@Eric-Arellano
Copy link
Collaborator Author

Eric-Arellano commented Mar 30, 2023

how about just legacy and then qiskit/core and qiskit/ecosystem? Then we don't use either pytorch or furo?

I don't think that makes sense because we want to have a folder for each base theme + its two variants. The structure looks like this:

qiskit_sphinx_theme
├── furo
│   ├── base
│   ├── ecosystem
│   └── terra
└── pytorch
    ├── base
    ├── ecosystem
    └── terra

It's an open question what to call the top-level folders & what to call terra.

@Eric-Arellano Eric-Arellano added this to the Theme v12.0.0 milestone Apr 11, 2023
@Eric-Arellano Eric-Arellano removed this from the Theme v1.12.0 milestone Apr 13, 2023
@Eric-Arellano Eric-Arellano marked this pull request as draft April 13, 2023 17:17
@Eric-Arellano Eric-Arellano changed the title Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes [on hold] Add infrastructure for qiskit_legacy and qiskit_ecosystem_legacy themes Apr 13, 2023
@Eric-Arellano
Copy link
Collaborator Author

Converted to draft. I got Furo working faster than expected, so there's now a world in which we never have a Pytorch Ecosystem theme and go straight to Furo. This PR would then be unnecessary.

I'll sync with Luciano on our timelines for Furo & for the ecosystem variant.

Feel free to unsubscribe from this PR :)

@Eric-Arellano
Copy link
Collaborator Author

We're trying to land Furo before we need the ecosystem variant. I'll reopen this if it ends up being necessary.

A lot of the new infrastructure is now set up in #269

@Eric-Arellano Eric-Arellano deleted the ecosystem-theme-infra branch April 27, 2023 13:53
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.

3 participants