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

Implement improved demo build pipeline #1286

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

brownj85
Copy link
Contributor

@brownj85 brownj85 commented Dec 23, 2024

Context

This PR is the initial phase of a re-factor of the demo build process. The goal is to improve the organization of the QML repo, make it easier to use, improve build times and simplify the process of deploying to pennylane.ai. To that end, this PR introduces a command line tool qml which will allow us to abstract over the sphinx-build process, and acts as an extension point to introduce new tooling. The qml tool can be install by running pip install . in the repository root.

Changes

Introduce a new structure for the demo files that gives each demo its own directory, containing the metadata (metadata.json), runtime requirements (requirements.in), any resources used by the demo, as well as the demo file itself (demo.py). Since this structure is not directly compatible with sphinx or sphinx-gallery, we use the qml tool to orchestrate the build process. (The new demo files are stored in the demonstrations_v2 directory, to avoid interfering with the existing build).

The command qml build [demo names] does the following:

  • copies the requested demo(s) into staging directory _build/demonstrations, structured to be compatible with sphinx-gallery
  • if execution is requested (--execute), install execution dependencies for requested demo(s). The package versions are constrained by the executable-dependencies dependency group in pyproject.toml
  • calls sphinx-build on the staging directory

Testing

Testers should verifying that the qml build command executes correctly using json and html formats, and produces output identical to the make html or make json commands. Testers should also run qml build with and without the --execute flag.

Installation

Create a Python3.10 virtual environment and install the qml tool:

qml $ python3.10 -m venv .venv
qml $ source .venv/bin/activate
qml (.venv) $ pip install -e .

Then qml build can be run anywhere in the repo:

qml build --format {html, json} [--execute]

Copy link

@anthayes92 anthayes92 left a comment

Choose a reason for hiding this comment

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

Looks like a great improvement, nice work @brownj85! 🚀

I've reviewed the additions to lib/qml/* here. A few questions and suggestions.

Are the tutorials mainly copy code? Probably too much to be reviewed in a PR, perhaps this can be QA'd interactively?

lib/qml/context.py Outdated Show resolved Hide resolved
lib/qml/app.py Outdated Show resolved Hide resolved
lib/qml/lib/cmds.py Outdated Show resolved Hide resolved
lib/qml/lib/cmds.py Outdated Show resolved Hide resolved
if quiet:
cmd.append("--quiet")

cmd.extend(str(arg) for arg in args)

Choose a reason for hiding this comment

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

Question

Should we include some error handling for processing the args here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we can just delegate to the underlying process to raise an appropriate error.

lib/qml/lib/demo.py Outdated Show resolved Hide resolved
lib/qml/lib/demo.py Show resolved Hide resolved
Comment on lines +144 to +145
def _install_build_dependencies(venv: Virtualenv, build_dir: Path):
"""Install dependencies for running sphinx-build into `venv`."""

Choose a reason for hiding this comment

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

Question

For my understanding - is this all being performed on a GitHub runner or are we executing this on some other hardware?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be run in a github runner, for the time being. We may look into moving demo execution into software cloud.

lib/qml/lib/demo.py Show resolved Hide resolved
lib/qml/lib/virtual_env.py Show resolved Hide resolved
@brownj85 brownj85 changed the title Sc 79849 implement improved demo build pipeline Implement improved demo build pipeline Jan 6, 2025
@brownj85 brownj85 requested a review from Alan-eMartin January 10, 2025 19:31
Copy link

@Alan-eMartin Alan-eMartin left a comment

Choose a reason for hiding this comment

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

Question:
@brownj85 — Do you have any suggestions on how we can test this locally?

Or as @anthayes92 mentioned above, should we do a live QA of these changes?

@brownj85
Copy link
Contributor Author

Hey @Alan-eMartin, @anthayes92 added some suggestions on testing to the top comment.

Copy link

@Alan-eMartin Alan-eMartin left a comment

Choose a reason for hiding this comment

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

Thanks for the presentation yesterday @brownj85, looks great.

@rashidnhm
Copy link
Collaborator

Would it make sense to use the qml CLI tool in the CI builds now, or would that be introduced in a subsequent PR?

@brownj85
Copy link
Contributor Author

brownj85 commented Jan 27, 2025

Would it make sense to use the qml CLI tool in the CI builds now, or would that be introduced in a subsequent PR?

That will be in a a subsequent PR - the diff for this is already massive.

Copy link

Thank you for opening this pull request.

You can find the built site at this link.

Deployment Info:

  • Pull Request ID: 1286
  • Deployment SHA: 00f517ce15815f960c77699147f7ba1c1cb64b9c
    (The Deployment SHA refers to the latest commit hash the docs were built from)

Note: It may take several minutes for updates to this pull request to be reflected on the deployed site.

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.

4 participants