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

added ci-daily #6177

Merged
merged 12 commits into from Jul 6, 2023
Merged

added ci-daily #6177

merged 12 commits into from Jul 6, 2023

Conversation

jackreeceejini
Copy link
Contributor

@jackreeceejini jackreeceejini commented Jun 30, 2023

New Cirq dev here, please advice on any improvements. Fixes #6130

@jackreeceejini jackreeceejini requested review from a team, vtomole and cduck as code owners June 30, 2023 18:52
@CirqBot CirqBot added the size: S 10< lines changed <50 label Jun 30, 2023
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please move Dockerfile updates to a separate PR.

Also please adjust the check/pytest arguments.

- name: Run Quil dependencies
run: docker-compose -f cirq-rigetti/docker-compose.test.yaml up -d
- name: Pytest check
run: check/pytest -n auto -m daily --ignore=cirq-core/cirq/contrib --rigetti-integration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the -m daily option, it happens to deselect all tests because we don't have the test marker daily defined.

Also please remove -n auto because it is already provided as a default in check/pytest.

@@ -26,8 +26,9 @@
# PR.
########################################################################################

FROM nikolaik/python-nodejs:python3.7-nodejs14
FROM nikolaik/python-nodejs:python3.9-nodejs16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert all Dockerfile-s and submit their updates as a new PR.
The same for cirq-all-no-contrib.txt unless it is needed for ci-daily.

It is better if we have PRs focused to a single topic only, it makes them easier to review and/or to revert should they introduce any problem.

@jackreeceejini
Copy link
Contributor Author

jackreeceejini commented Jul 6, 2023 via email

@jackreeceejini jackreeceejini reopened this Jul 6, 2023
@jackreeceejini
Copy link
Contributor Author

Eventually reverted the changes and hope all is clean. Any advice is welcome. Thanks.

@pavoljuhas
Copy link
Collaborator

I have a question, sorry if it sounds too naive. I am new to collaborating with GitHub, so I would like to ask if there is an efficient way of reverting the PRs and then submitting separate PRs since my local code is already synced to my public GitHub. I use vscode for development.

Please see the quick start doc at https://docs.github.com/en/get-started/quickstart/contributing-to-projects.
I keep the master branch in my clone/fork of cirq the same as in the main quantumlib:cirq repository, and
then I create new branches (starting at master) with descriptive names for each topic to be submitted as a PR.

Example:

$ git checkout -b add-ci-daily master
# ... work on the CI, create some commits
$ git push my-github-fork add-ci-daily
# ... submit PR from the add-ci-daily branch

# new topic - new branch
$ git checkout -b update-dockerfiles master
# ... work on Dockerfile-s create some commits
$ git push my-github-fork update-dockerfiles
# ... submit second PR from the update-dockerfiles branch

@jackreeceejini
Copy link
Contributor Author

jackreeceejini commented Jul 6, 2023 via email

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please fix a couple of format issues from yamllint, otherwise it looks good.
Thank you for contributing!

diff --git a/.github/workflows/ci-daily.yml b/.github/workflows/ci-daily.yml
index 39ae1465..6a466974 100644
--- a/.github/workflows/ci-daily.yml
+++ b/.github/workflows/ci-daily.yml
@@ -14,14 +14,14 @@ jobs:
     name: Pytest Ubuntu
     strategy:
       matrix:
-        python-version: ['3.9', '3.10' ]
+        python-version: ['3.9', '3.10']
     runs-on: ubuntu-20.04
     steps:
       - uses: actions/checkout@v3
       - uses: actions/setup-python@v4
         with:
-            python-version: ${{ matrix.python-version }}
-            architecture: 'x64'
+          python-version: ${{ matrix.python-version }}
+          architecture: 'x64'
       - uses: actions/cache@v2
         with:
           path: ${{ env.pythonLocation }}

@pavoljuhas pavoljuhas enabled auto-merge (squash) July 6, 2023 19:25
@pavoljuhas pavoljuhas merged commit 8a41b4a into quantumlib:master Jul 6, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add daily CI test for a fresh cirq --pre installation
3 participants