Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 171 171
Lines 11550 11563 +13
Branches 1511 1513 +2
=========================================
+ Hits 11550 11563 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| from braket.tasks.quantum_task import TaskResult, TaskSpecification | ||
| from braket.tasks.quantum_task_batch import QuantumTaskBatch | ||
|
|
||
| _TaskInputsGateDefinitions = tuple[ |
There was a problem hiding this comment.
Should this be a type alias?
Also, since you're making a type, maybe comment what these two dicts are supposed to be so that the type can be understood without searching around.
Separate question: why not use a dataclass here?
| **aws_quantum_task_kwargs (Any): Arbitrary kwargs for `QuantumTask`., | ||
| """ # noqa: E501 | ||
| tasks_inputs_gatedefs = AwsQuantumTaskBatch._tasks_inputs_gatedefs( | ||
| task_specifications, inputs, gate_definitions |
There was a problem hiding this comment.
I like the idea of having more structure for the task spec since there are so many parameters all being passed around.
The task_specification is essentially the program description, right? (like OpenQASM or ProgramSet)
I think it's out of scope to address this here -- but it does feel a little backwards. It seems as though a TaskSpecification should be a container with all the task parameters, including inputs and gate defs, and maybe shots and device arn for that matter.
Instead we have this "task program and inputs and gatedefs" container. It should probably be called the TaskSpecification but that name is already taken.
What do you think? Would it be better to later refactor all the parameters together into a single container, but for now keep these parameters separate and remove _TaskInputsGateDefinitions?
| aws_quantum_task_mock.return_value = task_mock | ||
|
|
||
| shots = [10, 20, 30] | ||
| device("arn:aws:braket:::device/quantum-simulator/amazon/sim").run_batch( |
There was a problem hiding this comment.
So, this doesn't use ProgramSets at all, right? It creates 3 separate single tasks?
The code doesn't touch the relevant lines so it's hard to tell from this change alone.
| max_parallel=1, | ||
| ) | ||
|
|
||
| normalize_mock.assert_called_once() |
There was a problem hiding this comment.
Not checking the result? What is this test validating?
| max_parallel=1, | ||
| ) | ||
|
|
||
| assert batch.size == len(shots) |
There was a problem hiding this comment.
Isn't this trivial from the test construction? What is this testing? The size method wasn't added in this PR, right?
| task_mock.state.return_value = "COMPLETED" | ||
| mock_create.return_value = task_mock | ||
|
|
||
| with pytest.raises(ValueError, match="Multiple shots and quantum tasks"): |
There was a problem hiding this comment.
This doesn't sound like a useful error message. Shouldn't it be "mismatch in the number of shots and tasks" or similar?
|
|
||
|
|
||
| @patch("braket.aws.aws_quantum_task.AwsQuantumTask.create") | ||
| def test_retry_with_shot_sequence(mock_create): |
There was a problem hiding this comment.
This looks like it's testing retry instead of the new shots logic. Can you help me understand how these two processes interact?
2 participants
Issue #, if available:
Description of changes:
Testing done:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.