-
Notifications
You must be signed in to change notification settings - Fork 191
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
Port QML Build to GitHub Actions with parallelisation #488
Conversation
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.
Thank you @rashidnhm! I took a first pass and added some comment. In general, I think we need more comments on what is going on with the smaller items in the workflows and the python script.
Also, could we add a readme file with the overall behaviour and describing how caching will work wrt the different branches?
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.
Looks great to me @rashidnhm! Left various questions, but in general I'm happy for this to be merged once @smokingroosters approves.
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.
Hi @rashidnhm, had a first pass on the first yaml file. Overall, the logic checks out, more commenting and documenting would be appreciated. Will continue with the second half of the PR.
Thank you all for all the useful feedback. I've tried my best to action each of them best I could. Docstrings have been added for all the python functions and I've added further comments in the build-pr workflow as well to outline what the steps do. Please take a look again and let me know of any further questions. |
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.
Hi @rashidnhm, made another pass, the docs and docstrings helped a lot! 🎉
I'd like to make a final pass, no blockers so far though.
What is the URL for the built website again?
I tried https://qml-build previews.pennylane.ai/pull_request_build_preview/488/index.html
though the space between qml-build
and previews
seems to cause an issue.
Every PR will get it's own deployment, the URL outlined in the description of this PR shows what that URL will look like.
That was a typo ... fixed 😄 ... and this PR won't exist in S3 as deploy-pr will only run from default 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.
Amazing work @rashidnhm, I'm excited to have this in!
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.
Looks good 💪 🥇
Would we plan to remove the CircleCI actions completely?
Ideally I think it would be best to consolidate all workflows to GH Actions... That being said I'd definitely wait and give this workflow a good soak period to ensure there aren't any major issues (and clear out any minor bugs). The final decision I leave to you, Josh and Alberto |
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.
@rashidnhm could we maybe add a small README/doc anywhere to document the caching behaviour?
Hello, currently making some of the changes from the review. Will update shortly. I will be adding a quick link to GitHub's documentation around caching, but it doesn't make sense for us to maintain a doc ourselves as the use case of caching here are actually simple. The caching behaviour is explained very well by GitHub themselves in their docs. In case caching behaviour changes it would be better for future readers to see GitHub's documentation versus maintain our own copy of it (that would need up-keeping). |
… in github_job_scheduler to be more readable
…be more relevant and explicit
… to in order to cancel previous run when new commits are pushed to a PR
…ar and explicit in relation to their functionality
Title: Port QML Docs pull request build process to GitHub Actions
Summary:
This pull request adds GitHub Actions workflows to build the QML docs for pull request and deploy them as static sites to S3.
Each pull request will get their own site under the URL prefix of:
The build workflow also implements parallelisation similar to pytorch/tutorials. Multiple workers are spawned each of which build a subsection of tutorials.
In order for this to work with forked pull request, the build and deploy workflows were separated and only the deploy workflow will get access to secrets.
Note: The setting up of workflow secrets and all AWS infrastructure has been setup with terraform
Relevant references:
Possible Drawbacks:
Depending on the volume of active pull request, we may hit an upper limit of workers available to the repository at a time. This will not cause any jobs to fail, it will queue the job until a runner is available.
Related GitHub Issues:
N/A