feat: universal QPU - #206
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis change adds universal QPU support and broad GraphQL schema extensions. It negotiates program formats, submits provider jobs, normalizes results, and adds catalog, workspace, competition, provider-job, resource, and storage entities. ChangesUniversal QPU provider flow
GraphQL schema extensions
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant QPU
participant Client
participant Provider
participant ResultDecoder
QPU->>Client: request platform input formats
Client-->>QPU: return advertised formats
QPU->>QPU: detect and encode programs
QPU->>Provider: submit serialized payload
Provider-->>QPU: return job metadata
QPU->>Provider: retrieve completed result
Provider-->>ResultDecoder: provide serialized result
ResultDecoder-->>QPU: return normalized counts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
test/test_universal_qpu.py (3)
693-694: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the fake measurement map as a
ClassVar.Ruff reports RUF012 for the mutable class attribute.
♻️ Proposed change
class CirqResult: - measurements = {"m": [[0, 1], [1, 0], [0, 1]]} + measurements: ClassVar[dict[str, list[list[int]]]] = { + "m": [[0, 1], [1, 0], [0, 1]] + }Add
from typing import ClassVarto the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_universal_qpu.py` around lines 693 - 694, Update the CirqResult test fixture’s mutable measurements class attribute to use a ClassVar annotation, and add the corresponding ClassVar import from typing.Source: Linters/SAST tools
550-551: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind the property access to silence Ruff B018.
The attribute access is the action under test, but Ruff reports it as a useless expression.
♻️ Proposed change
with pytest.raises(LookupError, match="nexus:Nope"): - qpu.input_formats + _ = qpu.input_formats🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_universal_qpu.py` around lines 550 - 551, Bind the qpu.input_formats property access in the pytest.raises block to a throwaway variable so Ruff B018 recognizes it as intentional, while preserving the expected LookupError and match assertion.Source: Linters/SAST tools
569-573: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
QPUJob.from_idandQPUJob.counts.The suite covers submission and
ProviderResult.counts, but not the two newQPUJobentry points.from_idbuilds aQPUfromqualified_platform_name(payload.get("platform"))and returnsserialization_format is None;countsmaps every result. TheFakeClient.sendhandler already returns aplatformobject forquery ProviderJob, so both are cheap to test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_universal_qpu.py` around lines 569 - 573, Add tests in test_job_reports_the_negotiated_format or the surrounding QPU job tests covering QPUJob.from_id and QPUJob.counts: verify from_id constructs the QPU using the payload platform and leaves serialization_format unset, and verify counts maps every returned result. Reuse the existing FakeClient.send query ProviderJob response and established test fixtures.python/aqora/_provider/formats.py (1)
296-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
strict=Trueto thezipcall.Ruff flags B905 on Line 303.
encodersis built fromsources, so the lengths always match, andstrict=Truedocuments that invariant.♻️ Proposed change
programs = [ encoder(source.program) - for encoder, source in zip(encoders, sources) + for encoder, source in zip(encoders, sources, strict=True) if encoder is not None ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/aqora/_provider/formats.py` around lines 296 - 305, Update the zip call in the serialization loop to pass strict=True, preserving the existing encoder/source pairing while documenting that encoders and sources always have equal lengths.Source: Linters/SAST tools
python/aqora/qpu.py (1)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
job._payloadreaches into a private attribute ofjobs.submit_model's return value.
QPUJobis constructed with the private_payloadof the submitted job. A rename insidepython/aqora/_provider/jobs.pybreaks this call site silently at runtime. Consider exposing a public accessor onProviderJob, for example apayloadproperty, and using it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/aqora/qpu.py` around lines 99 - 104, Replace the private job._payload access in the QPUJob construction flow with a public payload accessor on ProviderJob. Add a payload property to ProviderJob that returns the submitted payload, then pass job.payload from the surrounding submission method while preserving the existing serialization_format behavior.python/aqora/__init__.py (1)
11-11: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe new import creates an order-dependent import cycle.
aqora/__init__.pyimports.qpu, andpython/aqora/qpu.pyLine 7 importsClientback fromaqora. This resolves only becauseClientis already bound in the partially initializedaqoramodule when Line 11 runs. Any reordering of the imports above Line 11 raisesImportErrorat package import.Importing
Clientfrom its defining module inqpu.pyremoves the cycle. Note thattest/test_universal_qpu.pysetsaqora.Clienton a fake module, so a change here needs the test fake updated as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/aqora/__init__.py` at line 11, Update qpu.py to import Client directly from its defining module instead of through the aqora package, removing the order-dependent cycle introduced by aqora/__init__.py importing QPU and QPUJob. Update the fake aqora module setup in test_universal_qpu.py so it provides the direct Client dependency expected by qpu.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/aqora/_provider/client.py`:
- Line 126: Add inputFormats to the local ProviderPlatformMeta schema definition
used by the ProviderPlatforms query, or gate the meta.inputFormats selection so
it is omitted when unsupported by older servers; ensure the query remains valid
against the local schema and preserves compatibility with servers lacking this
field.
In `@python/aqora/_provider/results.py`:
- Around line 71-95: Update _cudaq_register_counts to validate the payload type
and available elements before every name, outcome, and triplet access. Reject
non-list/integer data and truncated or malformed records with an explicit
payload-validation error that identifies the CUDA-Q provider result, instead of
allowing IndexError or TypeError to escape; preserve normal decoding for valid
arrays.
- Around line 192-196: Update _pytket_counts to group each outcome’s bits
according to the registers in result.to_backend_result().c_bits, join bits
within each register, and separate register groups with spaces before
constructing the counts dictionary. Preserve counts and existing behavior for
single-register results.
---
Nitpick comments:
In `@python/aqora/__init__.py`:
- Line 11: Update qpu.py to import Client directly from its defining module
instead of through the aqora package, removing the order-dependent cycle
introduced by aqora/__init__.py importing QPU and QPUJob. Update the fake aqora
module setup in test_universal_qpu.py so it provides the direct Client
dependency expected by qpu.py.
In `@python/aqora/_provider/formats.py`:
- Around line 296-305: Update the zip call in the serialization loop to pass
strict=True, preserving the existing encoder/source pairing while documenting
that encoders and sources always have equal lengths.
In `@python/aqora/qpu.py`:
- Around line 99-104: Replace the private job._payload access in the QPUJob
construction flow with a public payload accessor on ProviderJob. Add a payload
property to ProviderJob that returns the submitted payload, then pass
job.payload from the surrounding submission method while preserving the existing
serialization_format behavior.
In `@test/test_universal_qpu.py`:
- Around line 693-694: Update the CirqResult test fixture’s mutable measurements
class attribute to use a ClassVar annotation, and add the corresponding ClassVar
import from typing.
- Around line 550-551: Bind the qpu.input_formats property access in the
pytest.raises block to a throwaway variable so Ruff B018 recognizes it as
intentional, while preserving the expected LookupError and match assertion.
- Around line 569-573: Add tests in test_job_reports_the_negotiated_format or
the surrounding QPU job tests covering QPUJob.from_id and QPUJob.counts: verify
from_id constructs the QPU using the payload platform and leaves
serialization_format unset, and verify counts maps every returned result. Reuse
the existing FakeClient.send query ProviderJob response and established test
fixtures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ab846d8-bd55-4af4-9d06-e6901102ae46
📒 Files selected for processing (9)
pyproject.tomlpython/aqora/__init__.pypython/aqora/_provider/client.pypython/aqora/_provider/formats.pypython/aqora/_provider/results.pypython/aqora/_provider/wire.pypython/aqora/qpu.pysrc/python_module.rstest/test_universal_qpu.py
a05b5e7 to
fd7e750
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
test/test_universal_qpu.py (2)
255-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the provider modules explicitly instead of reading
sys.modules.The
formats,wire, andresultsfixtures assume that loadingaqora.qpualready imported eachaqora._provider.*module. Ifqpu.pylater imports one of them lazily, the fixture raisesKeyErrorand many tests fail with a message that does not point at the cause.Use
importlib.import_moduleso the fixture states its own dependency.♻️ Proposed change
`@pytest.fixture` def formats(qpu_mod): - return sys.modules["aqora._provider.formats"] + return importlib.import_module("aqora._provider.formats") `@pytest.fixture` def wire(qpu_mod): - return sys.modules["aqora._provider.wire"] + return importlib.import_module("aqora._provider.wire") `@pytest.fixture` def results(qpu_mod): - return sys.modules["aqora._provider.results"] + return importlib.import_module("aqora._provider.results")Add the import at the top of the file:
import importlib🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_universal_qpu.py` around lines 255 - 267, Update the formats, wire, and results fixtures to explicitly load their provider modules with importlib.import_module instead of indexing sys.modules, and add the importlib import required by those fixtures. Preserve each fixture’s existing module target and return value.
126-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for paginated results.
FakeClient.sendalways returnsself.result_pages[0]withhasNextPage: False.result_pagesis a list of pages, andresultCountsums every page, so the fake can never exercise the cursor loop in the client. A job with more results than one page is a realistic case for a QPU run.Return page N based on the
aftercursor and sethasNextPageaccordingly, then add a test that spans two pages.Also applies to: 175-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_universal_qpu.py` around lines 126 - 128, Update FakeClient.send to select the page using the request’s after cursor, return that page’s results, and set hasNextPage based on whether another result_pages entry remains. Add a test covering a QPU job whose results span two pages, verifying the client follows the cursor and combines both pages.python/aqora/_provider/formats.py (1)
331-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the message distinguish an empty platform preference list.
encodefalls back to the sources' native formats whenacceptedis empty. In that case the message still says "the platform accepts ...", which names formats the platform never advertised. Pass a flag or the originalacceptedlist so the text can say that the platform stated no preference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/aqora/_provider/formats.py` around lines 331 - 334, Update the error-message construction in encode to retain whether the original accepted list was empty, and use that state to say the platform stated no preference instead of claiming it accepts the wanted formats. Preserve the existing wording for non-empty platform preferences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/aqora/_provider/formats.py`:
- Around line 110-124: Update _qasm_format to skip OpenQASM block comments,
including multi-line /* ... */ comments, before evaluating the first meaningful
line. Track whether scanning is inside a block comment, ignore comment content
and blank lines, then preserve the existing OPENQASM version detection and None
behavior for unsupported input.
In `@python/aqora/_provider/results.py`:
- Around line 101-102: Validate each packed outcome before assigning it in the
counts-building logic: if packed.bit_length() exceeds width, raise ValueError
and do not store the key. Otherwise preserve the existing
bin(packed)[2:].zfill(width) conversion and count assignment.
In `@schema.graphql`:
- Line 2490: Align eventCompetitionPinned with pinEventCompetition by making the
pinned lookup event-scoped and accepting the relevant eventId, or alternatively
enforce a single global pin with replacement semantics and document that
behavior in the schema. Keep the query and mutation scope consistent.
- Around line 3465-3470: Replace the GraphQL Int types used for storage byte
counts with the project’s 64-bit or custom wide scalar. Update
StorageStatus.used, capacity, available, and requested at
schema.graphql:3465-3470, and StorageUsage.used and limit at
schema.graphql:3478-3481, using the same scalar consistently at both sites.
In `@test/test_universal_qpu.py`:
- Around line 548-551: Update test_unknown_platform_raises so the intentional
qpu.input_formats access is assigned to a throwaway local variable inside the
pytest.raises block, preserving the LookupError trigger while satisfying Ruff
B018.
---
Nitpick comments:
In `@python/aqora/_provider/formats.py`:
- Around line 331-334: Update the error-message construction in encode to retain
whether the original accepted list was empty, and use that state to say the
platform stated no preference instead of claiming it accepts the wanted formats.
Preserve the existing wording for non-empty platform preferences.
In `@test/test_universal_qpu.py`:
- Around line 255-267: Update the formats, wire, and results fixtures to
explicitly load their provider modules with importlib.import_module instead of
indexing sys.modules, and add the importlib import required by those fixtures.
Preserve each fixture’s existing module target and return value.
- Around line 126-128: Update FakeClient.send to select the page using the
request’s after cursor, return that page’s results, and set hasNextPage based on
whether another result_pages entry remains. Add a test covering a QPU job whose
results span two pages, verifying the client follows the cursor and combines
both pages.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea52bf36-dfed-4b1b-890a-db6f2a6968f3
📒 Files selected for processing (12)
.github/workflows/ci.yamlpyproject.tomlpython/aqora/__init__.pypython/aqora/_provider/client.pypython/aqora/_provider/formats.pypython/aqora/_provider/results.pypython/aqora/_provider/wire.pypython/aqora/qpu.pyschema.graphqlsrc/python_module.rstest/test_pytket_provider.pytest/test_universal_qpu.py
🚧 Files skipped from review as they are similar to previous changes (6)
- python/aqora/init.py
- python/aqora/_provider/client.py
- pyproject.toml
- python/aqora/_provider/wire.py
- src/python_module.rs
- python/aqora/qpu.py
| type StorageStatus { | ||
| used: Int | ||
| capacity: Int | ||
| available: Int | ||
| requested: Int | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema size =="
wc -l schema.graphql || true
echo "== storage status/usage definitions =="
rg -n "type Storage(Status|Usage)|Scalars|Decimal|BigInt|Long|Byte|ByteCount|storage" schema.graphql
echo "== relevant schema section =="
sed -n '3430,3495p' schema.graphql
echo "== GraphQL Int scalar usage in schema =="
rg -n "scalar Int|type .*Int|: Int|`@default`|`@default`" schema.graphql | head -200Repository: aqora-io/cli
Length of output: 17309
🌐 Web query:
GraphQL spec Int signed 32-bit scalar maximum 2147483647 scalar types
💡 Result:
The GraphQL specification explicitly defines the Int scalar type as a signed 32-bit integer, with a valid range of -(2^31) to 2^31 - 1, which corresponds to the inclusive range of -2,147,483,648 to 2,147,483,647 [1][2][3]. Key details regarding the Int scalar include: - Strict Specification: GraphQL services must adhere to this definition; a service must not use the Int type to represent 64-bit numbers or other data outside this range [4][2][3]. - Error Handling: If an internal value or input value falls outside this 32-bit range (less than -2^31 or greater than or equal to 2^31), the service is required to raise an execution or request error [5][2][3]. - Alternatives for Larger Numbers: Because not all platforms and transport protocols support integers larger than 32 bits, the GraphQL specification recommends that numeric integer values exceeding this range be handled by using either the String type or a custom-defined scalar type (e.g., a "Long" or "BigInt" scalar) [6][2][3]. This limitation is intentionally maintained to ensure cross-platform compatibility, as many environments and clients—including those relying on JavaScript—have historically had varying or limited support for 64-bit integers [6][7].
Citations:
- 1: https://github.com/graphql/graphql-js/blob/main/src/type/scalars.ts
- 2: https://spec.graphql.org/October2021/
- 3: https://github.com/graphql/graphql-spec/blob/main/spec/Section%203%20--%20Type%20System.md
- 4: graphql/graphql-spec@39f7a34
- 5: graphql/graphql-spec@fdc2b81
- 6: Support of
Long(64-bit integer) scalar graphql/graphql-spec#73 - 7: Large Integers and GraphQLInt graphql/graphql-js#292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== defined scalars in schema =="
rg -n '^scalar ' schema.graphql || true
echo "== Int scalar definition overrides =="
rg -n 'scalar Int|Int =|Int scalar|GraphQLScalarType|new GraphQLScalarType' .Repository: aqora-io/cli
Length of output: 434
Use a wide scalar for storage byte counts.
GraphQL Int is signed 32-bit and cannot represent storage values above 2,147,483,647 bytes.
schema.graphql#L3465-L3470: ChangeStorageStatus.used,capacity,available, andrequestedto a 64-bit/custom wide scalar.schema.graphql#L3478-L3481: ChangeStorageUsage.usedandlimitto the same wide scalar.
📍 Affects 1 file
schema.graphql#L3465-L3470(this comment)schema.graphql#L3478-L3481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@schema.graphql` around lines 3465 - 3470, Replace the GraphQL Int types used
for storage byte counts with the project’s 64-bit or custom wide scalar. Update
StorageStatus.used, capacity, available, and requested at
schema.graphql:3465-3470, and StorageUsage.used and limit at
schema.graphql:3478-3481, using the same scalar consistently at both sites.
fd7e750 to
b5c9d28
Compare
1 participant
Summary by CodeRabbit