search

imr-framework / pypulseq

Pulseq in Python
https://pypulseq.readthedocs.io
GNU Affero General Public License v3.0
111 stars 58 forks source link

Add continous deployment (CD) workflow #173

Closed schuenke closed 4 weeks ago

schuenke commented 3 months ago

The last release and deployment to PyPi was more than a year ago and the version on PyPi is not even stable due to the np.float issue. This was fixed in #115 , but never released and I am 100% sure that 1) we loose a lot of (potential) users and 2) it leads to many unnecessary Issues and PRs (e.g. #119 , #138 , #172 ...)

Maybe even more important, it prohibits using pypulseq as dependency for other projects on PyPi, because PyPi doesn't allow direct dependencies and using the pypulseq version from PyPi leads to failing test pipelines. Just for me, it's two packages that I cannot release to PyPi atm.

I added a simple deployment workflow (similar to the example on python.org) that will deploy every tag push to PyPi. Meaning only if we add a Tag, it will be released.

Atm, I would vote for a 1.4.0.post1 tag (see my comment on versioning below as well)

For the workflow to work, @sravan953 only has to add GitHub as a trusted publisher for the pypulseq PyPi project as explained here: https://docs.pypi.org/trusted-publishers/adding-a-publisher/

This is done in 30 seconds!

If we want, we can also configure it to release every push to TestPyPi. If this is wanted, @sravan953 has to add GitHub as trusted publisher for the TestPyPi project as well. https://test.pypi.org/project/pypulseq/ does exist already.

I think it would also make sense at some point to become independent of the pulseq / pulseq for Siemens version. We can document somewhere which Pulseq versions are supported, but using the same version number doesn't really make sense IMHO for an independent project like PyPulseq.

On top, this might be a good timepoint to rename the default branch back to "main" 😄

FrankZijlstra commented 2 months ago

I am not so familiar with the github deployment scripts that I can review the commit itself.

However, I do fully agree with that it is necessary to push regular updates to PyPi. Especially since the last version a lot of bugs have been fixed, and the current dev version of PyPulseq appears to be working bug-free in most common scenarios. It would also help with teaching events that we don't have to instruct people to pip install from Github.

I think if we review our code properly for each PR, then publishing every push should not be a problem, but I also like the idea of using tags to have a bit more control over what gets published when.

On the naming, I think we should keep following the version number of the Pulseq file format (so not the interpreter or Pulseq package), as in the next major release there will be changes to the format (adding RF use and first/last fields for arbitrary gradients), which will not be compatible with older versions. So while the difference between 1.4.0 for PyPulseq vs 1.4.2 for Pulseq is not a big deal, it would be a big deal if we name a release 1.5.0 ahead of the 1.5.0 release of Pulseq which may include this change of file format.

schuenke commented 2 months ago

On the naming, I think we should keep following the version number of the Pulseq file format (so not the interpreter or Pulseq package), as in the next major release there will be changes to the format (adding RF use and first/last fields for arbitrary gradients), which will not be compatible with older versions. So while the difference between 1.4.0 for PyPulseq vs 1.4.2 for Pulseq is not a big deal, it would be a big deal if we name a release 1.5.0 ahead of the 1.5.0 release of Pulseq which may include this change of file format.

This does mean that we will deviate from the classical semantic versioning scheme using Major.Minor.Patch, because it would mean we could only add new features (this should increase Minor), when a new Pulseq file format is released, but I think breaking with these semantic versioning rules is not super problematic.

What exact versioning scheme do we use than: MAJOR.MINOR.PATCH from the current Pulseq version and than just increasing post-release numbers like post1, post2, post3 etc ?!

This would mean the current version would be 1.4.0.post1 ?!

Or do we support everything from pulseq 1.4.2 and our next release would be 1.4.2?

Or do we ignore the Pulseq Patch version and just use MAJOR.MINOR from Pulseq and than our own Patch numbers for PyPulseq? This would mean the next release is 1.4.1 😄

FrankZijlstra commented 2 months ago

I think with the way we're currently developing, the last suggestion (MAJOR.MINOR from Pulseq, our own patch number) makes the most sense. Right now I don't even know what exactly has changed in 1.4.1 and 1.4.2 in Pulseq, and it's tricky to find out exactly. We may have already implemented everything to be compatible with either 1.4.1 or 1.4.2? But setting ourselves up to follow all Pulseq features exactly is probably not a good idea. The important thing is to follow the file format specification to avoid confusion between Pulseq and PyPulseq versions.