OpenQuantum support#1169
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![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 (1)
qbraid/runtime/openquantum/provider.py (1)
205-212: Don't swallow enrichment failures silently.If the detail fetch starts failing, this quietly drops
constraint_dataand leaves partially populated profiles with no breadcrumb. Catch the specific client error and log the backend id instead ofexcept Exception: pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qbraid/runtime/openquantum/provider.py` around lines 205 - 212, Replace the broad except/pass around the get_backend_class_details call with handling for the specific client error thrown by the session (e.g., catch the session/client library's specific exception class instead of Exception) and log a warning that includes the backend id (device_id) and the caught exception; keep falling back to the basic device data but ensure the log message references self.session.get_backend_class_details(device_id) and that you still do not re-raise for non-fatal enrichment failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qbraid/runtime/openquantum/device.py`:
- Around line 146-163: The selection logic in device.py (symbols:
execution_plan_id, queue_priority_id, quote, plan["queue_priorities"],
plan["price"], q["price_increase"]) currently picks the cheapest plan first
which can pick a plan that doesn't offer the requested priority or not minimize
total cost; change it to select a valid plan+priority pair: if execution_plan_id
is provided, find that plan and then validate/choose the priority in
plan["queue_priorities"]; if only queue_priority_id is provided, first filter
quote for plans that contain that queue_priority_id and select among them; if
neither is provided, choose the plan and queue_priority pair that minimizes
total_cost = plan.get("price", 0) + q.get("price_increase", 0); raise ValueError
if a requested execution_plan_id or queue_priority_id cannot be found in the
filtered set.
In `@qbraid/runtime/openquantum/job.py`:
- Around line 79-88: The result() method in OpenQuantumJob accesses
self.device.id which raises when the job was loaded standalone without a device;
update result() (in class OpenQuantumJob / method result) to safely handle
missing device by checking for the attribute or using getattr(self, "device",
None) and only reading .id when device is not None, otherwise set device_id to
an empty string; ensure no other direct uses of self.device occur in result() so
standalone loaded jobs return properly.
- Around line 90-114: The code currently forwards the entire job_data via
enhanced_job_data into Result(...), which can leak transport fields like
output_data_url into Result.details; instead build a whitelist of allowed public
fields (e.g., explicitly pick/assign execution_plan, queue_priority and any
other safe metadata you intend to expose) and pass only those explicit keys to
Result (or set them as explicit keyword args) rather than **enhanced_job_data;
update the area around enhanced_job_data,
execution_plan_reverse/queue_priority_reverse and the Result(...) call to
construct and use a filtered_payload (or explicit kwargs) so sensitive transport
fields are never included.
---
Nitpick comments:
In `@qbraid/runtime/openquantum/provider.py`:
- Around line 205-212: Replace the broad except/pass around the
get_backend_class_details call with handling for the specific client error
thrown by the session (e.g., catch the session/client library's specific
exception class instead of Exception) and log a warning that includes the
backend id (device_id) and the caught exception; keep falling back to the basic
device data but ensure the log message references
self.session.get_backend_class_details(device_id) and that you still do not
re-raise for non-fatal enrichment failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3528f8ec-f40a-4a99-a78d-b921f9b7653b
📒 Files selected for processing (8)
CHANGELOG.mdqbraid/runtime/__init__.pyqbraid/runtime/loader.pyqbraid/runtime/openquantum/__init__.pyqbraid/runtime/openquantum/device.pyqbraid/runtime/openquantum/job.pyqbraid/runtime/openquantum/provider.pyqbraid/runtime/result.py
| # Plan & priority selection (your logic, slightly cleaner) | ||
| if execution_plan_id: | ||
| plan = next((p for p in quote if p["execution_plan_id"] == execution_plan_id), None) | ||
| if not plan: | ||
| raise ValueError(f"Execution plan '{execution_plan_id}' not found in quote.") | ||
| else: | ||
| plan = min(quote, key=lambda p: p.get("price", float("inf"))) | ||
|
|
||
| if queue_priority_id: | ||
| prio = next( | ||
| (q for q in plan["queue_priorities"] if q["queue_priority_id"] == queue_priority_id), | ||
| None, | ||
| ) | ||
| if not prio: | ||
| raise ValueError(f"Queue priority '{queue_priority_id}' not found in selected plan.") | ||
| else: | ||
| prio = min(plan["queue_priorities"], key=lambda q: q.get("price_increase", float("inf"))) | ||
|
|
There was a problem hiding this comment.
Select the cheapest valid plan/priority combination, not the cheapest plan first.
This fallback can overcharge users and can also reject a valid request. If queue_priority_id is set without execution_plan_id, Line 152 may choose a plan that doesn't offer that priority even though another quoted plan does. Even with no inputs, minimizing plan.price before price_increase is not the same as minimizing total cost.
Proposed fix
- if execution_plan_id:
- plan = next((p for p in quote if p["execution_plan_id"] == execution_plan_id), None)
- if not plan:
- raise ValueError(f"Execution plan '{execution_plan_id}' not found in quote.")
- else:
- plan = min(quote, key=lambda p: p.get("price", float("inf")))
-
- if queue_priority_id:
- prio = next(
- (q for q in plan["queue_priorities"] if q["queue_priority_id"] == queue_priority_id),
- None,
- )
- if not prio:
- raise ValueError(f"Queue priority '{queue_priority_id}' not found in selected plan.")
- else:
- prio = min(plan["queue_priorities"], key=lambda q: q.get("price_increase", float("inf")))
+ candidates = []
+ for candidate_plan in quote:
+ if execution_plan_id and candidate_plan["execution_plan_id"] != execution_plan_id:
+ continue
+ for candidate_prio in candidate_plan.get("queue_priorities", []):
+ if queue_priority_id and candidate_prio["queue_priority_id"] != queue_priority_id:
+ continue
+ total_price = candidate_plan.get("price", float("inf")) + candidate_prio.get(
+ "price_increase", float("inf")
+ )
+ candidates.append((total_price, candidate_plan, candidate_prio))
+
+ if not candidates:
+ raise ValueError(
+ "No matching execution plan / queue priority combination was returned in the quote."
+ )
+
+ _, plan, prio = min(candidates, key=lambda item: item[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Plan & priority selection (your logic, slightly cleaner) | |
| if execution_plan_id: | |
| plan = next((p for p in quote if p["execution_plan_id"] == execution_plan_id), None) | |
| if not plan: | |
| raise ValueError(f"Execution plan '{execution_plan_id}' not found in quote.") | |
| else: | |
| plan = min(quote, key=lambda p: p.get("price", float("inf"))) | |
| if queue_priority_id: | |
| prio = next( | |
| (q for q in plan["queue_priorities"] if q["queue_priority_id"] == queue_priority_id), | |
| None, | |
| ) | |
| if not prio: | |
| raise ValueError(f"Queue priority '{queue_priority_id}' not found in selected plan.") | |
| else: | |
| prio = min(plan["queue_priorities"], key=lambda q: q.get("price_increase", float("inf"))) | |
| # Plan & priority selection (your logic, slightly cleaner) | |
| candidates = [] | |
| for candidate_plan in quote: | |
| if execution_plan_id and candidate_plan["execution_plan_id"] != execution_plan_id: | |
| continue | |
| for candidate_prio in candidate_plan.get("queue_priorities", []): | |
| if queue_priority_id and candidate_prio["queue_priority_id"] != queue_priority_id: | |
| continue | |
| total_price = candidate_plan.get("price", float("inf")) + candidate_prio.get( | |
| "price_increase", float("inf") | |
| ) | |
| candidates.append((total_price, candidate_plan, candidate_prio)) | |
| if not candidates: | |
| raise ValueError( | |
| "No matching execution plan / queue priority combination was returned in the quote." | |
| ) | |
| _, plan, prio = min(candidates, key=lambda item: item[0]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qbraid/runtime/openquantum/device.py` around lines 146 - 163, The selection
logic in device.py (symbols: execution_plan_id, queue_priority_id, quote,
plan["queue_priorities"], plan["price"], q["price_increase"]) currently picks
the cheapest plan first which can pick a plan that doesn't offer the requested
priority or not minimize total cost; change it to select a valid plan+priority
pair: if execution_plan_id is provided, find that plan and then validate/choose
the priority in plan["queue_priorities"]; if only queue_priority_id is provided,
first filter quote for plans that contain that queue_priority_id and select
among them; if neither is provided, choose the plan and queue_priority pair that
minimizes total_cost = plan.get("price", 0) + q.get("price_increase", 0); raise
ValueError if a requested execution_plan_id or queue_priority_id cannot be found
in the filtered set.
There was a problem hiding this comment.
I think this is a valid note can you take a look?
There was a problem hiding this comment.
@ryanhill1, yes, the concern is valid on two counts:
-
Greedy plan-then-priority fails when only
queue_priority_idis specified. The current code picksmin(quote, key=lambda p: p.get("price", ...))first, locking in a plan before checking whether it even contains the requested priority. If the cheapest plan doesn't offer that priority but another plan does, the user gets an unnecessaryValueError. -
Independent minimization ≠ minimum total cost. Even with no user constraints, choosing the cheapest base price plan and then the cheapest price increase within it can produce a higher combined cost than a different (plan, priority) pair.
The proposed fix resolves both by:
- Iterating over all
(candidate_plan, candidate_priority)pairs in one pass. - Filtering by
execution_plan_idand/orqueue_priority_idwhen provided. - Selecting the pair that minimizes
plan["price"] + prio["price_increase"]. - Raising a single, clear
ValueErrorif no matching combination exists.
Here's the recommended replacement (same as the suggestion):
candidates = []
for candidate_plan in quote:
if execution_plan_id and candidate_plan["execution_plan_id"] != execution_plan_id:
continue
for candidate_prio in candidate_plan.get("queue_priorities", []):
if queue_priority_id and candidate_prio["queue_priority_id"] != queue_priority_id:
continue
total_price = candidate_plan.get("price", float("inf")) + candidate_prio.get(
"price_increase", float("inf")
)
candidates.append((total_price, candidate_plan, candidate_prio))
if not candidates:
raise ValueError(
"No matching execution plan / queue priority combination was returned in the quote."
)
_, plan, prio = min(candidates, key=lambda item: item[0])This correctly handles all four cases: both IDs specified, only one specified, or neither specified.
| def result(self) -> Result: | ||
| """Return the result of the OpenQuantum job.""" | ||
| self.wait_for_final_state() | ||
| job_data = self.session.get_job(self.id) | ||
| if job_data.get("status", "") != "Completed": | ||
| raise QbraidRuntimeError(f"Job failed: {job_data.get('message')}") | ||
|
|
||
| results = self.session.download_job_output(self.id) | ||
| data = GateModelResultData(measurement_counts=results) | ||
| device_id = self.device.id if self.device else "" |
There was a problem hiding this comment.
Avoid self.device here when the job was loaded standalone.
QuantumJob.device raises when no device was attached. load_job(..., provider="openquantum") can create an OpenQuantumJob with only a job id/session, so Line 88 will fail before returning the downloaded result.
Proposed fix
- device_id = self.device.id if self.device else ""
+ device_id = self._device.id if self._device is not None else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qbraid/runtime/openquantum/job.py` around lines 79 - 88, The result() method
in OpenQuantumJob accesses self.device.id which raises when the job was loaded
standalone without a device; update result() (in class OpenQuantumJob / method
result) to safely handle missing device by checking for the attribute or using
getattr(self, "device", None) and only reading .id when device is not None,
otherwise set device_id to an empty string; ensure no other direct uses of
self.device occur in result() so standalone loaded jobs return properly.
There was a problem hiding this comment.
Can you test instantiating an OpenQauntumJob with no device and see if you can call job.result() without any issue? Or if this needs to be updated to use self._device. Alternatively does the job metadata contain any information about what device it was run on that would allow setting this field without requiring the device object?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check QuantumJob base class - specifically the device property
ast-grep --pattern 'class QuantumJob {
$$$
}'
rg -n "def device" qbraid/runtime/job.py -A 10
# Check _device attribute in QuantumJob
rg -n "_device" qbraid/runtime/job.py -A 3 -B 1
# Check get_job method in provider.py to see what fields are returned
rg -n "def get_job" qbraid/runtime/openquantum/provider.py -A 20
# Check the full result() method context
cat qbraid/runtime/openquantum/job.py
Length of output: 5965
🏁 Script executed:
#!/bin/bash
# Check create_job and prepare_job to understand job payload/response fields
rg -n "def create_job\|def prepare_job\|backend_class\|device_id\|backendClass" qbraid/runtime/openquantum/provider.py -A 15
# Check if job_data / get_job response ever references a backend or device id
rg -n "backend_class_id\|device_id\|backendClassId" qbraid/runtime/openquantum/ -r
Length of output: 414
🏁 Script executed:
#!/bin/bash
# Check create_job and prepare_job to understand job payload/response fields
rg -n "def create_job|def prepare_job" qbraid/runtime/openquantum/provider.py -A 20
# Check if job_data / get_job response ever references a backend or device id
rg -n "backend_class_id|device_id|backendClassId" qbraid/runtime/openquantum/
Length of output: 2969
@ryanhill1, I can confirm the bug is real. Here's a breakdown:
Why if self.device else "" doesn't work
QuantumJob.device is a property that raises ResourceNotFoundError (not returns None) when no device is attached:
# qbraid/runtime/job.py
`@property`
def device(self) -> qbraid.runtime.QuantumDevice:
"""Returns the qbraid QuantumDevice object associated with this job."""
if self._device is None:
raise ResourceNotFoundError("Job does not have an associated device.")
return self._deviceSo if self.device else "" never reaches the falsy branch on a standalone-loaded job — it raises instead.
Two possible fixes
Option 1 — Use self._device directly (quick fix):
device_id = self._device.id if self._device is not None else ""Option 2 — Derive device_id from the job metadata (better):
In device.py Line 118, the submit() method sends "backend_class_id": self.id in the job creation payload:
"backend_class_id": self.id,The scheduler API likely echoes this field back in the GET /v1/jobs/{job_id} response. If so, job_data already has the device identifier available without needing the device object at all:
device_id = job_data.get("backend_class_id", "")This would be the most robust approach since it works regardless of whether a device object was attached to the job, and uses the canonical identifier the API already tracks. It's worth verifying that backend_class_id is indeed present in the get_job response from the OpenQuantum scheduler.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ryanhill1
left a comment
ryanhill1
left a comment
There was a problem hiding this comment.
Thanks for the PR looks really good!
A few small housekeeping things in addition to my comments:
- Add unit tests with mocked API responses that satisfy codecov
- A few remote e2e tests that hit the actual server also good to have but not required
- Update
docs/api/qbraid.runtime.rstwith openquantum provider - Run
tox -e linters && tox -e format-checkand fix any errors.arguments-differ,import-outside-toplevel,broad-exception-caught, andtoo-many-argumentscan be ignored inline. But the rest, includingcyclic-importwill need to be resolved.
| # Plan & priority selection (your logic, slightly cleaner) | ||
| if execution_plan_id: | ||
| plan = next((p for p in quote if p["execution_plan_id"] == execution_plan_id), None) | ||
| if not plan: | ||
| raise ValueError(f"Execution plan '{execution_plan_id}' not found in quote.") | ||
| else: | ||
| plan = min(quote, key=lambda p: p.get("price", float("inf"))) | ||
|
|
||
| if queue_priority_id: | ||
| prio = next( | ||
| (q for q in plan["queue_priorities"] if q["queue_priority_id"] == queue_priority_id), | ||
| None, | ||
| ) | ||
| if not prio: | ||
| raise ValueError(f"Queue priority '{queue_priority_id}' not found in selected plan.") | ||
| else: | ||
| prio = min(plan["queue_priorities"], key=lambda q: q.get("price_increase", float("inf"))) | ||
|
|
There was a problem hiding this comment.
I think this is a valid note can you take a look?
| for i, item in enumerate(run_input) | ||
| ] | ||
|
|
||
| # Auto-select organization (your logic — perfect) |
There was a problem hiding this comment.
| # Auto-select organization (your logic — perfect) | |
| # Auto-select organization |
| if not queue_priority_id: | ||
| raise ValueError(f"Unknown queue priority: {queue_priority}") | ||
|
|
||
| # Plan & priority selection (your logic, slightly cleaner) |
There was a problem hiding this comment.
| # Plan & priority selection (your logic, slightly cleaner) | |
| # Plan & priority selection |
| def result(self) -> Result: | ||
| """Return the result of the OpenQuantum job.""" | ||
| self.wait_for_final_state() | ||
| job_data = self.session.get_job(self.id) | ||
| if job_data.get("status", "") != "Completed": | ||
| raise QbraidRuntimeError(f"Job failed: {job_data.get('message')}") | ||
|
|
||
| results = self.session.download_job_output(self.id) | ||
| data = GateModelResultData(measurement_counts=results) | ||
| device_id = self.device.id if self.device else "" |
There was a problem hiding this comment.
Can you test instantiating an OpenQauntumJob with no device and see if you can call job.result() without any issue? Or if this needs to be updated to use self._device. Alternatively does the job metadata contain any information about what device it was run on that would allow setting this field without requiring the device object?
| def __getattr__(self, name: str) -> Any: | ||
| """Allow access to details as attributes.""" | ||
| if name in self._details: | ||
| return self._details[name] | ||
| raise AttributeError(f"'{self.__class__.__name__}' object has no attribute '{name}'") | ||
|
|
There was a problem hiding this comment.
I'm not sure that we want to support retrieving details as attributes because they are not uniform across providers. Could cause confusion. Would rather leave top level Result method/fields/attrs consistent, and then provider-specific fields within result.details and result.data.
| - Added `OpenQuantumProvider`, `OpenQuantumDevice`, and `OpenQuantumJob` classes implementing the qBraid runtime interface for Open Quantum | ||
|
|
There was a problem hiding this comment.
If you could add a short working example that would be great.
May need to merge in latest from main and move it up to the new "Unreleased" section bc we release v0.12.0 since this PR was originally opened.
| ) | ||
|
|
||
| @cached_method | ||
| def get_devices(self, **kwargs) -> list[OpenQuantumDevice]: |
There was a problem hiding this comment.
| def get_devices(self, **kwargs) -> list[OpenQuantumDevice]: | |
| def get_devices(self) -> list[OpenQuantumDevice]: |
kwargs unused. can ignore pylint arguments differ.
2 participants
Summary of changes
Added
OpenQuantumProvider,OpenQuantumDevice, andOpenQuantumJobclasses implementing the qBraid runtime interface for Open QuantumSummary by CodeRabbit