Adding a Mock Test for MLIR Submissions#10
Conversation
|
|
||
| def mock_job_status(job_id, job_request): | ||
| """Mock job status to return a predefined status.""" | ||
| def mock_cancelled_status(job_id, job_request): |
There was a problem hiding this comment.
Why were these changes necessary for Pennylane mock tests?
There was a problem hiding this comment.
I have added this because i noticed that we have end_status JobStatus.CANCELLED in mqss_client.py and in the Pennylane MLIR mock we needed to handle the CANCELLED status properly. Therefore first i added test_pennylane_job_cancellation function and then to test it i added this function.
| """Mock job status to return a predefined status.""" | ||
| return JobStatus.COMPLETED | ||
|
|
||
| def mock_job_result(job_id, job_request): |
There was a problem hiding this comment.
Same point: Why were these changes necessary for Pennylane mock tests?
There was a problem hiding this comment.
This was not necessary for Pennylane. I added this because the one of the test for mqss_client_mock was failing with below error. Should i remove this?
test/test_mqss_client_mock.py::TestMQSSClientMock::test_wait_for_job_result[MQP] FAILED
test/test_mqss_client_mock.py::TestMQSSClientMock::test_wait_for_job_result[HPC] FAILED
AssertionError: assert {'0': 500, '1': 500} == {'00': 500, '11': 500}
| """Create a mock Pennylane job request.""" | ||
| return CircuitJobRequest( | ||
| resource_name="mock-backend", | ||
| circuits="mock-mlir-circuit", |
There was a problem hiding this comment.
The circuits should be the pennylane defined circuits (currently qasm, soon to be MLIR), here it is just a string as "mock-mlir-circuit"
There was a problem hiding this comment.
From the comments in another pull request [https://github.com//pull/8](Job request support for pennylane) i understood that for any new format only the value of the circuit attribute need to be changed therefore i used as a string.
Where can i find the details for "pennylane defined circuits"?
There was a problem hiding this comment.
I have updated the code to use QASM circuit definition from "test/example.qasm".
Bmete7
left a comment
Bmete7
left a comment
There was a problem hiding this comment.
The changes done in test_mqss_client_mock does not seem to be relevant to pennylane mock tests.
Also, a correct pennylane_job_request fixture has to be provided, that includes an actual expected pennylane circuit and not a string value. For this version, we can use a qasm, and later on we can also add an MLIR input.
|
Seems good to me. Adding @mnfarooqi for the final review. |
mnfarooqi
left a comment
mnfarooqi
left a comment
There was a problem hiding this comment.
At the MQSS client abstraction level, a job is a circuit in string(stream of bytes) format, regardless of whether it originates from Qiskit or Pennylane.
Could you please provide more information on the purpose of these mock tests, why they are necessary, and what additional functionality is tested here that is not tested in test_mqss_client_mock.py?
|
Even though the current format of a pennylane job is a circuit, it will be an MLIR code which will also get serialized, however, might have a different type than CircuitJobRequest, i.e., |
The MLIR code when serialized is still a CircuitJobRequest, as it is still a circuit but in a different language (format). If you want to implement serialisation as a part of this repo, then testing that functionality would be relevant. Then we would also need to modify the CircuitJobRequest and implement specialization like QiskitJobRequest/PennylaneJobRequest/MLIRJobRequests |
Should i then work on the test cases for serialisation? Sorry, but i am a bit lost here. |
As I mentioned, if @Bmete7 deems it feasible for the serialisation to be part of this repository, then you can add the serialisation and its test cases. However, if it is planned to be part of the Pennylane repository, then it is not needed here. |
|
After this discussion, I think it would make sense to handle these cases on the PennyLane part, as it seems that after serialization, the CircuitJobRequest objects are identical instead of their contents, which wasn't clear to me before. We might have to rethink this strategy as we move forward to the Catalyst integration with Pennylane Adapter, but I would say that until now, we can close this issue without merging and instead work on the PennyLane side of things. |
|
closing the pull request, as per @Bmete7 suggestion. |
3 participants
a mock test case, that receives jobs from Pennylane Adapter, and forwards the job (that has an MLIR) to a mock RabbitMQ server.