Repository navigation
Conversation
Introduce .github/labeler.yml as the shared rules config, wire actions/labeler for pull requests, and label new issues when they mention repository paths. Area labels are additive on PR updates (sync-labels disabled). Fixes quantumlib#8345
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8369 +/- ##
==========================================
- Coverage 99.59% 99.58% -0.01%
==========================================
Files 1137 1137
Lines 105134 105134
==========================================
- Hits 104703 104699 -4
- Misses 431 435 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Run black and added tests for |
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
This is a bit too much in a single PR.
Can you please hold on with issues labeling for a follow-up PR and have only the PR labeling here?
| .github/labeler.yml | ||
|
|
||
| - name: Label the PR with area labels | ||
| continue-on-error: true |
There was a problem hiding this comment.
This is redundant for a last step in a job:
| continue-on-error: true |
There was a problem hiding this comment.
I don't believe that's completely true. continue-on-error: true on a step does not only allow subsequent steps in the same job to execute; it also prevents a failing step from marking the enclosing job as failed. If continue-on-error: true is removed, any runtime or API error in actions/labeler itself will fail the job and produce a red check on the PR, which is undesirable because a failure to apply labels should not block a PR.
@pavoljuhas @rosspeili I think this should be put back.
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
Please see a couple of inline suggestions for pr-labeler.
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for this work.
I didn't know that actions/labeler is only meant to handle PRs. Having to maintain a separate script for parsing config files, doing the matching, doing the labeling, etc., is undesirable. Have you looked into possible alternatives to actions/labeler that might work for both issues and PRs? Looking around very briefly I found https://github.com/jimschubert/labeler-action but I don't know how good it is, and didn't do an exhaustive search of alterantives.
If it were possible to use an alternative action that works for both issues and PRs, and save the need to maintain a separate, that would be much preferred.
Remove issue labeling (custom script, issue-labeler job, and tests). Rename labeler config to .github/labeler.yaml with proper indentation, reviewer glob additions, and changed-files-labels-limit. Upgrade actions/labeler to v6.1.0 and drop redundant continue-on-error on the area label step.
|
Hey @pavoljuhas, @mhucka, thanks a lot for the review and feedback.
Issue auto-labeling can follow in a separate PR if needed. Lmk how this looks <3 |
|
Fixed a small triage.md wrap (line 95 was 103 chars, now uses a relative link, and under 100). Re labels limit @mhucka |
|
@mhucka I looked into labeler-action and a few other common setups from modern repos. labeler-action does handle both issues and PRs in one workflow, but it matches regex on title/body only, and not changed files or path globs, so it’s a poor fit for the pr What most large repos seem to do is split by signal type, not force one tool for everything:
If you ask me, there is not one action, one config, path globs for both option existing atm and hybrid is the viable alt to itself :D - happy to spike that in a small follow up after this lands, or awaiting further directions. |
Thank you for sharing the table of possible options. IMO, we should go for github/issue-labeler which is an official product and should have stringent security review. The labeling workflows run with elevated privileges of the It is also not too common practice to list affected paths in issue descriptions, so trying to use one path-based config for both PRs and issues may not work well enough. |
|
THanks a lot @pavoljuhas, and done: removed /labels/README.md, and applied the triage md suggestion, also updated |
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for this effort, and sorry for taking so long to circle back to this. This is looking good. Just a few adjustments left, and I think it'll be ready to merge.
Also, please update the title of the PR since this is now only about PRs.
| .github/labeler.yml | ||
|
|
||
| - name: Label the PR with area labels | ||
| continue-on-error: true |
There was a problem hiding this comment.
I don't believe that's completely true. continue-on-error: true on a step does not only allow subsequent steps in the same job to execute; it also prevents a failing step from marking the enclosing job as failed. If continue-on-error: true is removed, any runtime or API error in actions/labeler itself will fail the job and produce a red check on the PR, which is undesirable because a failure to apply labels should not block a PR.
@pavoljuhas @rosspeili I think this should be put back.
|
@rosspeili Coming back to this, I'd like to work on getting this in. Do you mind if I make the necessary changes and then push them to your branch? |
Add an extra `compresslevel` argument to `to_json_gzip` and change its default value from gzip module's 9 to 1 for fastest compression. This improves the speed of `to_json_gzip` by about 30% while the compression ratio w/r to plain JSON is increased from 2.3% to 3.4%. See the PR comments for associated benchmarks. Towards quantumlib#6110
…uantumlib#8483) `pyproject.toml` had an exclusion for mypy for setup.py, but the pattern looked like a mistake: ```toml [tool.mypy] exclude = [ '/setup\.py', 'cirq-google/cirq_google/cloud/', ] ``` The leading `/` meant that the pattern matched the 9 nested setup.py files (e.g., cirq-core/setup.py), but not the top-level setup.py. This seemed odd, and let me to think it was an accident. However, it turned out _not_ to be a mistake: it is on purpose, to avoid Mypy producing the error "Duplicate module name" when multiple setup.py files were passed to it. We do want Mypy to check the top-level setup.py, so the `exclude` must not exclude that one. To help future devs not go down the wrong rabbit hole, this PR adds a comment to clarify that the pattern is deliberate.
The list of modules under `tool.mypy.overrides` in `pyproject.toml` included several that no longer are used anywhere in the code base. They may have been left-overs from when `cirq-rigetti` was included. In addition, some other modules now bundle type annotations and thus do not need to be in the overrides list. This PR updates the list of overrides to remove the unnecessary entries.
…8490) Exclude output filename and modification time from gzip metadata. The compressed gzip data are otherwise binary reproducible for the same plain input.
… ms (quantumlib#8492) In `Calibration.timestamp_str`, `datetime.datetime.fromtimestamp` with `self.timestamp/1000` already includes millisecond precision in the resulting `datetime` object. Adding `self.timestamp % 1000000` via `datetime.timedelta(microseconds=...)` is incorrect: `self.timestamp` is in milliseconds (not microseconds), and its sub-second component was already accounted for by `fromtimestamp`. In `test_calibration_timestamp_str`, the fixture `_CALIBRATION_DATA` had `timestamp_ms: 1562544000021`. Because `1562544000` is a multiple of 1000, `1562544000021 % 1000000` evaluated to 21. That added 21 microseconds, turning `.021000` into `.021021` and masking the defect by embedding the buggy value directly into the test assertion. Testing a timestamp whose seconds part is not divisible by 1000 (e.g. `1723795877630`) exposed that the calculation erroneously added 877,630 microseconds (nearly a full second). This PR fixes the code by removing the extra time delta and updating the test. (This is also the approach suggested by the original author of issue quantumlib#8407, @qobserva-labs).
…b#8501) Internal test framework has a Python build for which nonzero gzip compresslevel-s produce the same output. This broke the test which assumed an improved compression at level 9 w/r to level 1. Since the purpose of `test_json_gzip_compresslevel` is to verify that `compresslevel` argument is plugged in, we just use level 0 (no compression) instead of 1, which should show a difference from level 9 for all Python builds. Applies downstream fix from cl/1012424547
…uantumlib#8499) Cirq's API documentation pages on quantumai.google are generated using [TensorFlow's doc generation system](https://github.com/tensorflow/docs) via Google-internal workflows. The `tensorflow_docs` module has several limitations that together cause the problem reported in issue quantumlib#4657: * It drops `*args`, `**kwargs`, and positional-only (`/`) type annotations. Its AST extractor ignores `vararg`, `kwarg`, and `posonlyargs`; this can be seen by inspecting the code in its `_ArgDefaultAndAnnotationExtractor.visit_FunctionDef`. * Even if `varargs` or `varkwargs` have annotations in the `annotation_source_dict` create by the extractor, `TfSignature.__str__` separates `VAR_POSITIONAL` and `VAR_KEYWORD` from regular parameters and formats them without calling its `formatter` function. * If arguments documented in a docstring are not formatted in the way the TF docs code expects, the signature formatting code will not use it. This happens because `TitleBlock._split_items` in the TF docs module's `parser.py` parses docstring lines purely via regex without consulting the `TfSignature` class object. * It doesn't handle `@typing.overload` methods with unannotated runtime definitions. Some Cirq methods place type annotations on `@overload` stubs while leaving the runtime implementation unannotated; because `inspect.signature` and `get_source.get_ast` inspect only the runtime function object, the generated doc have neither parameter nor return type annotations. The correct solution would be to fix the limitations in the upstream TensorFlow doc system code. Due to several factors, this may not be possible. So as an alternative, this PR patches Cirq's `dev_tools/docs/build_api_docs.py`, which is the code that configures and interfaces the TensorFlow doc software. By replacing the TF docs function signature visitors with enhanced alternatives that parse annotations and keyword arguments, we can make the system output the type hints that are currently missing. (Side note: I wrote this PR description; it is not LLM-generated.)
* Updates `actions/setup-python` from 7.0.0 to 7.1.0 * Updates `ossf/scorecard-action` from 2.4.4 to 2.5.0 * Updates `github/codeql-action/upload-sarif` from 4.38.3 to 4.39.1 Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
High-level explanation: When I was coding up `cirq.While`, a number of
comments came up during code review that looked like they also applied
for `cirq.If`, so I wanted to ensure that we didn't leave that hanging.
Note: I used Antigravity to get this list of changes and put it
together, so I want to call that out proactively.
More specifically:
- (requested by Mike) Docstring now says "experimental class" instead of
"experimental function".
- (requested by Pavol) `__str__` now indents multi-line sub-operations
so they line up (e.g. a `CircuitOperation` body). Before, the second and
third lines started at column 0.
- (requested by Pavol) `_is_parameterized_` passes the conditions tuple
straight to `protocols.is_parameterized` instead of looping over it with
`any(...)`.
- (requested by Mike) The diagram code now returns `NotImplemented` when
the sub-operation has no wire symbols (e.g. `cirq.If('a', []))`. I
removed the `# pragma: no cover` because that branch is now tested.
- (requested by Mike) `_qasm_` uses `protocols.QasmArgs()` instead of an
inline import.
- (requested by Mike) QASM bug fix: the old code only put the if in
front of the first statement when the sub-operation exported more than
one. `cirq.If('a', cirq.CCZ(...))` exports h; ccx; h, and only the first
h was conditioned. QASM 2.0 has no blocks, so each statement gets its
own if (...) prefix, the same way ClassicallyControlledOperation does
it.
`if_op_test.py` changes to be consistent with `while_op_test.py`:
- Added multi-line `__str__` cases, with one condition and with two.
- Added `test_diagram_empty_op_tree`, run for both [] and
cirq.Circuit().
- Updated `test_qasm` for the new QASM 3.0 { } output and added a
nested-If case.
- Added `test_qasm_multi_statement_sub_operation` (CCZ, checked in QASM
2.0 and 3.0).
- Added a CircuitOperation-body case to the existing
`test_qasm_sub_op_no_qasm`.
…ib#8503) The latest pytest-benchmark-5.3.1 includes bugfix that makes it work with pytest-9.2. Here we unpin pytest and require the fixed pytest-benchmark instead. Ref: https://github.com/ionelmc/pytest-benchmark/blob/v5.3.1/CHANGELOG.rst Follow-up to quantumlib#8489
* Bump black from 26.5.1 to 26.10.1 * Bump ruff from 0.16.10 to 0.17.2 * Bump pylint to 4.1.3 and switch to exact version specification * Update qiskit-aer to ~=0.18.0 to permit the latest version --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Bumps the non-security group with 2 updates: - Updates `@types/three` from 0.185.4 to 0.186.0 - Updates `vite` from 8.3.3 to 8.3.5 * Bump undici from 7.30.0 to 7.30.1 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This reduces some of the redundancy in the `.github/CODEOWNERS` file and makes more extensive use of GitHub Teams. After updating the memberships of the teams and the CODEOWNERS rules, it became apparent that listing individual users was unnecessary and would merely add maintenance work; in addition, the people in question have not reviewed PRs for a long time. So, I removed individual users from direct mention in CODEOWNERS. I also removed the special cases for docs/noise/qcvv and cirq-core/cirq/experiments. Cirq-maintainers must be listed for those in any case, and after removing the individual users, having separate entries for those file paths turned out to be redundant because those people were in Cirq-maintainers. Finally, I created a new GitHub team, Cirq Google Contributors, for people who are familiar with the more Google-specific code and tutorials, and listed this team for cirq-google and docs/tutorials/google/. (This GitHub team follows the pattern of Cirq Pasqal Contributors, Cirq IonQ Contributors, etc.)
…lib#8506) This updates some of the links in the top-level README.md file, adds TNCO to the table of related projects, and removes the OpenSSF badget from the top. (The OpenSSF list of conditions include some low-value items that we are not likely to be satisfy, which means our score will always be a bit low. Let's just drop it.)
old: https://github.com/quantumlib/cirq new: https://github.com/quantumlib/Cirq And also list it consistently with the "https" protocol.
…mlib#8491) 1. `SetVariable._from_json_dict_` passes the JSON `expression` to `SetVariable.__init__`, which runs `sympy.sympify` on any value that is not already a `sympy.Basic`. 2. `sympify` parses with sympy's eval-based parser, so a circuit read by `cirq.read_json` whose `expression` is a bare string reaches that parser and can run arbitrary code, the same vector `boolean_hamiltonian._parse_boolean_expr` already guards against. Real serialized output is always a structured sympy object, so `_from_json_dict_` now rejects a non-`Basic` expression and the `sympify` string convenience stays for in-process construction. --------- Co-authored-by: Pavol Juhas <juhas@google.com>
Part of quantumlib#8374. This PR replaces the simulator `RandomState` with `np.random.Generator` using the new `parse_random_generator` function. This had far-reaching consequences in the tests as many test results were hard-coded to a specific `RandomState` seed and therefore broke. The failing tests were fixed in several ways: * In some cases, tests checked a statistical result with too narrow a range but a fixed seed. In these cases, the seed was removed and the range broadened or the samples increased. * In some cases the stream of random numbers from a RandomState with a given seed was checked against a hardcoded sequence of numbers. In these cases, the test was replaced with a test that runs a given seed twice and compares the result. However one hardcoded stream test was kept to ensure portability of the random stream. * `fidelity_estimation_test` was left using `RandomState` as the removal of the seed-dependence would require more in-depth changes. Some potential ways this could break existing workflows: * Any output from the simulator that depends on randomness will change. * `np.random.seed` will no longer seed the simulator (`cirq.value.reseed_default_prng(seed)` should be used instead). * Existing tests could become flaky due to changing random streams. If this happens, offending tests should be fixed with the `@retry_once_with_later_random_values` decorator. This PR was assisted by AI.
We don't want a failure to label to block a PR.
Many changes.
|
@rosspeili In an effort to move this along, I pushed a number of changes to your branch – I hope that's okay! This was in an effort to update the file glob pattern. It addresses all my previous individual comments plus makes further adjustments. It does not address @pavoljuhas's comment about which GitHub Actions action to use – I'm still working on that matter. My current goal with this push was to figure out the pattern globs and bring the PR up to the latest commits on main. |
|
Here is the outcome of a dry-run with the labeling rules defined in the latest
|
For PR labeling, |
Add path-based
area/*andinterface/*auto-labeling for pull requests viaactions/labelerv6.1..github/labeler.yaml(path globs, aligned with CODEOWNERS).github/workflows/pr-labeler.yamlchanged-files-labels-limit: 5to avoid spam on wide refactorssync-labels: false, area labels are additive; manually added labels are not strippedIssue auto-labeling is out of scope for this PR. Per review, a follow-up can use
github/issue-labeler for issue triage labels
(separate from path-based PR rules).
Fixes #8345 (PR portion).