feat: scaleway integration#198
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR upgrades the CLI and provider to Python 3.10, adds a Qiskit provider (client, backend, job/result adapter, primitives), extends the GraphQL schema for provider/rewards/announcements/location, and adds tests plus CI steps. ChangesPython 3.10 Migration and Qiskit Provider Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb1f5b39c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
1-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege GitHub token permissions for this workflow.
This workflow currently uses default
GITHUB_TOKENpermissions, which can be broader than needed. Pinning permissions reduces CI blast radius with no functional downside for these steps.Suggested fix
name: Test on: pull_request: merge_group: push: branches: - main + +permissions: + contents: read jobs:🤖 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 @.github/workflows/ci.yaml around lines 1 - 13, Add explicit least-privilege GITHUB_TOKEN permissions to the workflow by inserting a top-level permissions block (e.g., under the existing "name: Test") that restricts scopes to only what the "test" job needs (for typical CI runs start with "permissions: contents: read" and add only other minimal scopes such as "checks: write" or "actions: read" if a specific step requires them); update the permissions mapping to be as narrow as possible for the "test" job referenced by the "jobs: test" block and adjust further only when a step fails due to missing token scope.Source: Linters/SAST tools
🤖 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_cli/qiskit/backend.py`:
- Around line 52-55: QPU.__init__ currently calls Target.from_configuration(...,
num_qubits=0) which advertises a zero‑qubit backend; change that to use
num_qubits=None so the Target is treated as abstract/unbounded per Qiskit 2.3.
Locate the Target.from_configuration call in QPU.__init__ and replace the
num_qubits argument value 0 with None (i.e., num_qubits=None) to prevent
transpilation/validation from rejecting real circuits.
In `@python/aqora_cli/qiskit/client.py`:
- Around line 143-161: The HTTP helpers _put_json_payload and _download_text
call urllib.request.urlopen() without timeouts; modify both to pass a sensible
timeout (e.g., a constant DEFAULT_HTTP_TIMEOUT) to urlopen so uploads and
downloads cannot block indefinitely. Update _put_json_payload (where Request is
created) and the urlopen call in _download_text to include the timeout argument,
and add the timeout constant (or optional parameter) near these functions so
callers/config can adjust if needed; keep behavior otherwise unchanged and
ensure tests still read ETag from response.headers in _put_json_payload.
- Around line 226-243: The pagination loop using PROVIDER_JOB_RESULTS_QUERY and
the variables node, results, page_info, and after must guard against
non-advancing cursors: after each fetch, check that page_info["endCursor"] is
present and different from the previous after; if endCursor is missing or
unchanged (i.e., page_info["endCursor"] is None or equals the current after),
raise a RuntimeError or LookupError with a clear message (including job_id and
the problematic cursor) to fail fast instead of looping indefinitely; update the
code around the while True block that calls self._client.send(...) to perform
this validation before assigning after = page_info["endCursor"].
In `@python/aqora_cli/qiskit/job.py`:
- Around line 55-63: The single-result branch of QPUJob.result() returns
results[0] directly and therefore ignores the job_id override passed into the
call; update that branch to apply the override just like the multi-result merge
does: if job_id is provided, set/override Result.job_id on the single Result (or
create a copy/dict and set "job_id") before returning. Locate the early-return
that checks if len(results) == 1 and ensure it mutates or replaces results[0] to
include the supplied job_id so single-item and multi-item behavior is
consistent.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 1-13: Add explicit least-privilege GITHUB_TOKEN permissions to the
workflow by inserting a top-level permissions block (e.g., under the existing
"name: Test") that restricts scopes to only what the "test" job needs (for
typical CI runs start with "permissions: contents: read" and add only other
minimal scopes such as "checks: write" or "actions: read" if a specific step
requires them); update the permissions mapping to be as narrow as possible for
the "test" job referenced by the "jobs: test" block and adjust further only when
a step fails due to missing token scope.
🪄 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
Run ID: 078e36bf-350a-4ea6-a00d-f13f8ff5a3b5
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/cd.yaml.github/workflows/ci.yamlCargo.tomlpyproject.tomlpython/aqora_cli/qiskit/__init__.pypython/aqora_cli/qiskit/_deps.pypython/aqora_cli/qiskit/backend.pypython/aqora_cli/qiskit/client.pypython/aqora_cli/qiskit/job.pypython/aqora_cli/qiskit/primitives.pyschema.graphqltest/test_qiskit_provider.py
jpopesculian
force-pushed
the
jpop/scaleway
branch
from
eb1f5b3 to
280a5d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/aqora/qiskit/client.py (1)
260-273:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against non-advancing cursors in provider-platform pagination.
Line 272 can loop forever if
hasNextPage=TruebutendCursoris null or unchanged.Suggested change
while True: @@ page_info = connection["pageInfo"] if not page_info["hasNextPage"]: return platforms - after = page_info["endCursor"] + next_cursor = page_info.get("endCursor") + if not next_cursor or next_cursor == after: + raise RuntimeError( + f"Provider platforms pagination returned a non-advancing cursor: {next_cursor!r}" + ) + after = next_cursor🤖 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/qiskit/client.py` around lines 260 - 273, The pagination loop in the provider-platform fetch (inside the while True that calls _run_sync with PROVIDER_PLATFORMS_QUERY) can hang if pageInfo.hasNextPage is True but pageInfo.endCursor is null or equals the previous cursor; update the loop to detect non-advancing cursors by storing the previous after value, and if page_info["endCursor"] is None or equals the previous after, break or raise a descriptive exception instead of continuing; ensure platforms are returned or an error is raised so the function (_client.send caller) cannot loop forever..github/workflows/ci.yaml (1)
10-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege
GITHUB_TOKENpermissions for the workflow.The workflow relies on default token permissions; this is broader than needed for this test job and weakens CI security posture.
Suggested patch
@@ on: pull_request: merge_group: push: branches: - main + +permissions: + contents: read🤖 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 @.github/workflows/ci.yaml around lines 10 - 13, The workflow currently uses default GITHUB_TOKEN permissions; add an explicit least-privilege permissions block at the top of the workflow (or under the workflow-level header) to restrict the token for the "test" job (named "Test Workspace"). Add a permissions map such as "permissions: contents: read" (and any other specific scopes your tests require, e.g., "actions: read" or "checks: write") so the token grants only the minimal rights needed for the Test Workspace job.Source: Linters/SAST tools
🧹 Nitpick comments (1)
python/aqora/qiskit/__init__.py (1)
9-20: ⚡ Quick winSort
__all__to satisfy Ruff RUF022.Line 9-20 is intentionally curated, but Ruff flags this as unsorted; if lint is enforced, this will keep failing CI.
Suggested change
__all__ = [ - "ProviderJobResultItem", - "QPU", - "QPUJob", - "QuantumCircuit", "backend", "client", "estimator", "job", "primitives", + "ProviderJobResultItem", + "QPU", + "QPUJob", + "QuantumCircuit", "sampler", ]🤖 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/qiskit/__init__.py` around lines 9 - 20, The __all__ list is unsorted and triggers Ruff RUF022; reorder the entries in the __all__ variable alphabetically (e.g., arrange "ProviderJobResultItem", "QPU", "QPUJob", "QuantumCircuit", "backend", "client", "estimator", "job", "primitives", "sampler" into proper alphabetical order) so the exported names are sorted and the linter passes; update the __all__ assignment (the list literal named __all__) accordingly.Source: Linters/SAST tools
🤖 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 `@install.py`:
- Line 49: The error message in install.py is misleading because
get_python_clib() only probes Python 3.10–3.12; update the logic so the behavior
and message match: either (A) change the error string in get_python_clib() to
explicitly state the supported shared-library range (e.g. "Python shared library
3.10–3.12 is required") or (B) expand the probe loop in get_python_clib() to
include newer minor versions you intend to support and then update the message
to reflect the new range (use get_python_clib() and its internal probe list to
locate where to change both probing bounds and the final error text).
In `@python/aqora/qiskit/backend.py`:
- Around line 44-56: The __init__ of the Aqora backend silently ignores url and
allow_insecure_host when a client is passed; add validation in __init__ to raise
a ValueError if client is provided together with url or allow_insecure_host (or
alternatively update the constructor docstring to clearly state the precedence),
referencing the parameters client, url, allow_insecure_host and the local
variable raw_client / AqoraGraphQLClient to locate the logic; implement the
validation at the start of __init__ so callers get an immediate, clear error
instead of silently ignored arguments.
In `@python/aqora/qiskit/client.py`:
- Around line 136-141: The _run_sync bridge currently blocks indefinitely on
future.result(), so modify _run_sync to call future.result(timeout=...) with a
reasonable default (e.g., configurable DEFAULT_SYNC_TIMEOUT) and handle
concurrent.futures.TimeoutError: on timeout cancel the future (future.cancel()),
optionally shut down or log via the same logger, and raise a clear timeout
exception to the caller; keep references to the existing symbols _run_sync,
run_factory, asyncio.run_coroutine_threadsafe, and _background_loop so the
change is localized and configurable.
In `@python/aqora/qiskit/job.py`:
- Line 37: Replace the truthiness checks that treat empty string errors as
absent: in ProviderJobResultItem.to_qiskit_result change the conditional "if
self.error:" to an explicit "if self.error is not None" (or equivalent) so an
empty string is preserved as an error, and in QPUJob._job_status change "if
error:" to "if error is not None" (or equivalent) so empty-string error payloads
are handled; update any related branches that assume falsy means no error to
rely on the explicit None check instead.
- Around line 119-125: In _job_status, the current truthy check "if error:" will
ignore empty-string errors; change it to an explicit None check (e.g., "if error
is not None") so that empty string errors are treated as errors; update the
_job_status function accordingly (mirroring the handling used in
ProviderJobResultItem.to_qiskit_result) to ensure any non-None error value,
including "", returns JobStatus.ERROR.
- Around line 36-42: In to_qiskit_result, the check `if self.error:` treats
empty strings as falsy and can skip real error payloads; change this to an
explicit None check (e.g., `if self.error is not None:`) so any non-None
error—including empty strings—raises the JobError, and keep the existing
result_url presence check and subsequent calls to graphql.download_text(...) and
QuantumProgramResult.from_json_str(...). Ensure you only alter the conditional
on self.error in the to_qiskit_result method to use `is not None` (or equivalent
explicit None check) and leave the rest of the flow unchanged.
In `@test/test_qiskit_provider.py`:
- Line 643: The tests use a bare property access inside pytest.raises
(qpu.target) which triggers Ruff B018; fix by first assigning the property to a
local variable (e.g., target = qpu.target) and then use that variable inside the
pytest.raises context at both occurrences where qpu.target is referenced (the
two pytest.raises blocks referencing qpu.target), so the exception check uses an
explicit variable rather than a bare attribute access.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 10-13: The workflow currently uses default GITHUB_TOKEN
permissions; add an explicit least-privilege permissions block at the top of the
workflow (or under the workflow-level header) to restrict the token for the
"test" job (named "Test Workspace"). Add a permissions map such as "permissions:
contents: read" (and any other specific scopes your tests require, e.g.,
"actions: read" or "checks: write") so the token grants only the minimal rights
needed for the Test Workspace job.
In `@python/aqora/qiskit/client.py`:
- Around line 260-273: The pagination loop in the provider-platform fetch
(inside the while True that calls _run_sync with PROVIDER_PLATFORMS_QUERY) can
hang if pageInfo.hasNextPage is True but pageInfo.endCursor is null or equals
the previous cursor; update the loop to detect non-advancing cursors by storing
the previous after value, and if page_info["endCursor"] is None or equals the
previous after, break or raise a descriptive exception instead of continuing;
ensure platforms are returned or an error is raised so the function
(_client.send caller) cannot loop forever.
---
Nitpick comments:
In `@python/aqora/qiskit/__init__.py`:
- Around line 9-20: The __all__ list is unsorted and triggers Ruff RUF022;
reorder the entries in the __all__ variable alphabetically (e.g., arrange
"ProviderJobResultItem", "QPU", "QPUJob", "QuantumCircuit", "backend", "client",
"estimator", "job", "primitives", "sampler" into proper alphabetical order) so
the exported names are sorted and the linter passes; update the __all__
assignment (the list literal named __all__) accordingly.
🪄 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
Run ID: 6d3a7329-c48b-43e9-a31c-cbde2e468274
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/cd.yaml.github/workflows/ci.yamlCargo.tomlREADME.mdinstall.pypyproject.tomlpython/aqora/_aqora.pyipython/aqora/qiskit/__init__.pypython/aqora/qiskit/_deps.pypython/aqora/qiskit/backend.pypython/aqora/qiskit/client.pypython/aqora/qiskit/job.pypython/aqora/qiskit/primitives.pysrc/python_module.rstemplate/src/use_case.rstest/test_qiskit_provider.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- .github/workflows/cd.yaml
- pyproject.toml
jpopesculian
force-pushed
the
jpop/scaleway
branch
3 times, most recently
from
58b6946 to
ccc26e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/aqora/qiskit/client.py (2)
255-277: 💤 Low valueBind loop variable in lambda to satisfy Ruff B023.
The lambda at line 261 captures
afterby reference, which Ruff flags as B023. While this works correctly here because_run_syncblocks until the lambda executes, using a default argument makes the intent explicit and prevents future surprises if the execution model changes.♻️ Proposed fix
while True: node = _run_sync( - lambda: self._client.send( + lambda after=after: self._client.send( PROVIDER_JOB_RESULTS_QUERY, id=job_id, first=page_size, after=after, ) )["node"]🤖 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/qiskit/client.py` around lines 255 - 277, The lambda passed to _run_sync around self._client.send captures the loop variable after by reference (Ruff B023); change the lambda to bind after as a default argument (e.g., lambda after=after: ...) so the current cursor value is captured at call-site when invoking PROVIDER_JOB_RESULTS_QUERY, leaving the rest of the pagination logic in the while loop (items, page_info, cursor, hasNextPage checks) unchanged.Source: Linters/SAST tools
287-302: 💤 Low valueSame loop variable binding issue.
Apply the same fix for consistency.
♻️ Proposed fix
while True: connection = _run_sync( - lambda: self._client.send( + lambda after=after: self._client.send( PROVIDER_PLATFORMS_QUERY, first=page_size, after=after, ) )["providerPlatforms"]🤖 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/qiskit/client.py` around lines 287 - 302, The loop captures the loop variable 'after' in the lambda passed to _run_sync, causing a late-binding bug; fix it by binding the current after value into the lambda's default arguments (e.g., change _run_sync(lambda: self._client.send(..., after=after)) to _run_sync(lambda after=after: self._client.send(..., after=after))) so each iteration uses the intended cursor when calling PROVIDER_PLATFORMS_QUERY and processing pageInfo/page nodes.Source: Linters/SAST tools
🤖 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 `@test/test_qiskit_provider.py`:
- Around line 313-315: FakeClient.s3_put currently lacks the content_type
parameter expected by AqoraGraphQLClient.upload_payload; update
FakeClient.s3_put signature to accept content_type: str = None (or **kwargs) and
store or ignore it as needed (e.g., append it to self.uploads) so calls like
AqoraGraphQLClient.upload_payload(..., content_type="application/json") don't
raise TypeError; if you choose to capture content_type for assertions, modify
the test assertion that inspects FakeClient.uploads (used in
test_qpu_run_uploads_qio_model) to unpack three values (url, body, content_type)
instead of two.
---
Nitpick comments:
In `@python/aqora/qiskit/client.py`:
- Around line 255-277: The lambda passed to _run_sync around self._client.send
captures the loop variable after by reference (Ruff B023); change the lambda to
bind after as a default argument (e.g., lambda after=after: ...) so the current
cursor value is captured at call-site when invoking PROVIDER_JOB_RESULTS_QUERY,
leaving the rest of the pagination logic in the while loop (items, page_info,
cursor, hasNextPage checks) unchanged.
- Around line 287-302: The loop captures the loop variable 'after' in the lambda
passed to _run_sync, causing a late-binding bug; fix it by binding the current
after value into the lambda's default arguments (e.g., change _run_sync(lambda:
self._client.send(..., after=after)) to _run_sync(lambda after=after:
self._client.send(..., after=after))) so each iteration uses the intended cursor
when calling PROVIDER_PLATFORMS_QUERY and processing pageInfo/page nodes.
🪄 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
Run ID: 64fa5976-55c8-4723-b72b-891b5c752b64
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/cd.yaml.github/workflows/ci.yamlCargo.tomlREADME.mdclient/src/s3.rsinstall.pypyproject.tomlpython/aqora/_aqora.pyipython/aqora/qiskit/__init__.pypython/aqora/qiskit/_deps.pypython/aqora/qiskit/backend.pypython/aqora/qiskit/client.pypython/aqora/qiskit/job.pypython/aqora/qiskit/primitives.pyschema.graphqlsrc/python_module.rstemplate/src/use_case.rstest/test_qiskit_provider.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/ci.yaml
- Cargo.toml
- template/src/use_case.rs
- install.py
- python/aqora/qiskit/init.py
- python/aqora/qiskit/primitives.py
- pyproject.toml
- .github/workflows/cd.yaml
- python/aqora/_aqora.pyi
- python/aqora/qiskit/backend.py
- python/aqora/qiskit/_deps.py
- python/aqora/qiskit/job.py
- schema.graphql
jpopesculian
force-pushed
the
jpop/scaleway
branch
from
ccc26e2 to
fcac7d7
Compare
jpopesculian
force-pushed
the
jpop/scaleway
branch
from
fcac7d7 to
acc1ed4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores