multi enviroment ci workflow#697
Conversation
Removed 'multiple_enviroments' branch from push triggers.
Refactor development dependencies for testing on Windows.
| include: | ||
| - os: windows-latest | ||
| python-version: "3.11.*" | ||
| test-group: | ||
| name: core | ||
| paths: "tests/circuit_tests tests/core_tests tests/primitives_tests tests/jax_tests tests/test_typing.py" | ||
| pip-extras: "[dev-windows,xdsl,iqm]" | ||
| pytest-flags: '-k "not test_catalyst_interface and not test_qubit_array_fusion"' | ||
|
|
||
| - os: windows-latest | ||
| python-version: "3.11.*" | ||
| test-group: | ||
| name: algorithms-and-integrations | ||
| paths: "tests/algorithms_tests tests/operators_tests tests/block_encodings_tests tests/interface_tests tests/stim_integration_tests" | ||
| pip-extras: "[dev-windows,xdsl,iqm]" | ||
| pytest-flags: '--ignore=tests/interface_tests/test_converter_qrisp_to_qml.py' | ||
|
|
||
| - os: windows-latest | ||
| python-version: "3.12.*" | ||
| test-group: | ||
| name: core | ||
| paths: "tests/circuit_tests tests/core_tests tests/primitives_tests tests/jax_tests tests/test_typing.py" | ||
| pip-extras: "[dev-windows,xdsl,iqm]" | ||
| pytest-flags: '-k "not test_catalyst_interface and not test_qubit_array_fusion"' | ||
|
|
||
| - os: windows-latest | ||
| python-version: "3.12.*" | ||
| test-group: | ||
| name: algorithms-and-integrations | ||
| paths: "tests/algorithms_tests tests/operators_tests tests/block_encodings_tests tests/interface_tests tests/stim_integration_tests" | ||
| pip-extras: "[dev-windows,xdsl,iqm]" | ||
| pytest-flags: '--ignore=tests/interface_tests/test_converter_qrisp_to_qml.py' |
There was a problem hiding this comment.
since a lot of these lines are repetitive, you could refactor them into unique lines where you do not have to write the python version by hand. python-version is defined before. The lines you added could directly use that. You also shouldn't add os: windows-latest multiple times. Add it once and then use it in the form of a strategy.
See https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/run-job-variations and https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#using-a-multi-dimensional-matrix
Also, I see certain OS are skipping some unit tests. I assume that module is incompatible with the OS? You should add a comment about each such that someone who edits the workflow in the future knows why the ignore flags were initially added.
Updated concurrency settings and enhanced matrix strategy for testing across multiple environments.
|
The refactoring is using a flattened nested object to exactly match the entire object structure. So that Windows can receive its custom overrides. Flat strings match perfectly every time. |
Removed 'multiple_enviroments' branch from push triggers.
| - os: macos-latest | ||
| pip-extras: "[dev,catalyst,xdsl,iqm]" | ||
| - os: windows-latest | ||
| pip-extras: "[dev-windows,xdsl,iqm]" |
There was a problem hiding this comment.
I am not too sure if dev-windows is correct here? In the toml file, the group you defined is dev_windows. I am mostly confused why the CI did not raise an error here. maybe I am missing something here 😅
There was a problem hiding this comment.
ah, this is because of name normalization...will correct it
- All underscores (_) and periods (.) are replaced with hyphens (-).
- All characters are converted to lowercase.
Maybe it's just me but I feel the |
|
I had tried this approach previously, but it had problems getting the flags, and it adds complexity to the |
| dev-testing-windows = [ | ||
| "pytest==9.1.0", | ||
| "qiskit-aer", | ||
| "pyscf", |
There was a problem hiding this comment.
simply curious: is pyscf incompatible with windows?
There was a problem hiding this comment.
it is, alongside catalyst.
There was a problem hiding this comment.
Good to know! Should we think about documenting this in the readme and the development guide as well?
There was a problem hiding this comment.
From what I can see, there is no documentation about installing Qrisp in a Windows environment, so I think this is a great suggestion. We can discuss it in the software engineering meeting.
can you help me understand this? how is it making the We can add a Then do something similar for the pytest step. |
|
Because we are putting the configuration inside the command when we can do it outside. Using While this approach could work, it looks more like a workaround. I think we need to do our best to be prepared for the future and follow the best DevOps practises.
Thank you very much for your nice and constructive feedback! |
|
Sorry if I am dragging this out. Not my intention.
Right. My main issue is not that you used Since the two steps below follow the same pattern, I wonder if utilizing a matrix makes sense here. Just thinking out loud, not suggesting you to make a change. |
|
That is a really great point, and I completely see what you mean about the visual redundancy! The reason it’s structured this way comes down to a known limitation in GitHub's matrix engine. If we define a default To make the Windows overrides actually stick, we have to define Let me know what you think! |
Do you mind if I take a couple of days reading up on the Github Actions documentation? As far as I remember, when you use In the past, I have been able to keep Edit: Below is what I have been suggesting. |
Not at all, we can continue the discussion until we find the best solution! |
Description
This workflow allows running the tests in MacOS and Windows environments.
Related Issues
Closes #
Related to #
Type of Change
Breaking Change?
If yes, describe the impact and migration path:
What was changed?
macos-latestandwindows-latestto the Pytest GitHub Actions workflow.pyproject.tomlto structure dependency groups cleanly and excludedpyscfon Windows.How was it tested?
Screenshots / Output (if applicable)
Checklist
Reviewer Notes