Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] Incorporate pulser-pasqal package in the pasqal-cloud repo #147

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

HGSilveri
Copy link
Contributor

@HGSilveri HGSilveri commented Nov 8, 2024

Description

  • Isolate pasqal_cloud package inside the pasqal-cloud directory
  • Add the pulser-pasqal directory with the pulser_pasqal package

Remaining Tasks

  • Include pulser-pasqal in the mypy checks
  • Share a centralized version between the two packages
  • Update the "Publish" workflow to release both packages simultaneously
  • Setup TestPyPI and PyPI to use trusted publishing
  • Update the repo README

Checklist

  • The title of the PR follows the right format: [{Label}] {Short Message}. Label examples: IMPROVEMENT, FIX, REFACTORING... Short message is about what your PR changes.

Versioning (PASQAL developers only)

  • Update the version of pasqal-cloud in VERSION.txt following the changes in your PR and by using semantic versioning.

Documentation

  • Update CHANGELOG.md with a description explaining briefly the changes to the users.

Tests

  • Unit tests have been added or adjusted.
  • Tests were run locally.

Internal tests pipeline (PASQAL developers only)

  • Update and run the internal tests while targeting the branch of this PR.
    If your PR hasn't changed any functionality, it still needs to be validated against internal tests.

After updating the version (PASQAL developers only)

  • Open a PR on the internal tests that updates the version used for the pasqal-cloud backward compatibility tests.

@HGSilveri
Copy link
Contributor Author

Note for reviewers: Reviewing one commit at a time is recommended on this one

@HGSilveri
Copy link
Contributor Author

I think this is ready for review now. I don't think any of these changes has a place in the CHANGELOG but let me know otherwise.

@HGSilveri HGSilveri marked this pull request as ready for review November 20, 2024 09:55
@oliver-gordon
Copy link
Collaborator

No, these don't reflect a direct change in the cloud release so changelog is OK without an update.

But this leaves me to assume you think we should bump the cloud version to twin with the pulser-pasqal lib in another MR?

if I've missed it please point me to it.

I think this looks OK

Copy link
Collaborator

@MatthieuMoreau0 MatthieuMoreau0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

As is, is this going to release new versions of pulser pasqal upon merging to main? Or are there extra steps needed (apart from bumping the version to 1.x to publish a newer version number)

@HGSilveri
Copy link
Contributor Author

HGSilveri commented Nov 25, 2024

@oliver-gordon @MatthieuMoreau0
The version is an issue I didn't make a decision on. As it stands,

  • The two packages are set to share the same version number and the publishing workflow is triggered by pushes to main, so this will attempt to publish both pasqal-cloud and pulser-pasqal to 0.20.1 simultaneously.
  • However, pulser-pasqal 0.20.1 already exists, so this will fail to some extent. Whether it would still succeed at publishing pasqal-cloud I don't know but I don't think we should test it.
  • So, I think we should update the version. My questions to you are then:
    • To what version?
    • Is it fine to have a release on the CHANGELOG without changes? If not, what should I fill in there?

EDIT: I guess we can also set the CI workflow to skip-existing: true for PyPI so that we publish only pasqal-cloud at 0.20.1

@Mildophin
Copy link
Collaborator

@oliver-gordon @MatthieuMoreau0 The version is an issue I didn't make a decision on. As it stands,

* The two packages are set to share the same version number and the publishing workflow is triggered by pushes to main, so this will _attempt_ to publish both `pasqal-cloud` and `pulser-pasqal` to 0.20.1 simultaneously.

* However, `pulser-pasqal` 0.20.1 already exists, so this will fail to some extent. Whether it would still succeed at publishing `pasqal-cloud` I don't know but I don't think we should test it.

* So, I think we should update the version. My questions to you are then:
  
  * To what version?
  * Is it fine to have a release on the CHANGELOG without changes? If not, what should I fill in there?

EDIT: I guess we can also set the CI workflow to skip-existing: true for PyPI so that we publish only pasqal-cloud at 0.20.1

I am wondering if the bump to 0.21.0 would be clearer for the previous user of pulser pasqal ? It is a bit a breaking change in some way. I am not sure though.

It is fine yes if there is no feature added (or one changed). I guess it would be great to have the documentation about how to use pulser-pasqal feature somewhere in it.

CHANGELOG.md Outdated Show resolved Hide resolved
pasqal-cloud/README.md Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@HGSilveri
Copy link
Contributor Author

I am wondering if the bump to 0.21.0 would be clearer for the previous user of pulser pasqal ? It is a bit a breaking change in some way. I am not sure though.

For the user, there is no breaking change since the library itself did not change. I think 0.20.2 makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants