pypulseq icon indicating copy to clipboard operation
pypulseq copied to clipboard

Conda Package for Pypulseq

Open gabuzi opened this issue 2 years ago • 23 comments

Is your feature request related to a problem? Please describe. Pypulseq is only available from PyPI. I frequently find myself using conda as a package manager because some performance critical, mostly GPU related packages are managed via conda environments in our organization.

While installing packages from PyPI via pip in a conda environment works generally ok, it is fragile and does not properly take dependencies into account. Thus, the recommended practice is to not run any conda commands after pip has been used.

A requirement of building conda packages is that they can only list other conda packages as dependencies, not packages from PyPI.

Describe the solution you'd like Build conda packages for every release. Conda-forge can be setup to automatically track PyPI releases. See e.g. https://github.com/conda-forge/sigpy-feedstock/pull/3 contributed by @wtclarke for sigpy.

The following conda meta.yaml file was obtained with the grayskull tool (https://conda-forge.org/blog/posts/2020-03-05-grayskull/), setup to create a conda package from the pypulseq package on PyPI and can serve as a starting point.

{% set name = "pypulseq" %}
{% set version = "1.4.0" %}  # this should eventually be changed to v 1.4.x where x is the version where sigpy was bumped to newer versions that support modern numpy!

package:
  name: {{ name|lower }}
  version: {{ version }}

source:
  url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/pypulseq-{{ version }}.tar.gz
  sha256: 0f639aa1bdbdba547b38da73cd59f54325f1586c84263ddac59973bc83acc74e

build:
  noarch: python
  script: {{ PYTHON }} -m pip install . -vv
  number: 0

requirements:
  host:
    - python >=3.6
    - pip
  run:
    - python >=3.6
    - coverage >=6.2
    - matplotlib-base >=3.5.2
    - numpy <1.24  # again, should be adjusted in line with scipy requirements
    - scipy >=1.8.1
    - sigpy ==0.1.25  # I bumped this to the latest available on conda-forge, but should be adjusted

test:
  imports:
    - pypulseq
  source_files:
    - pypulseq/tests
  commands:
#    - pip check  # disabled because checks for sigpy 0.1.23!
    - pytest pypulseq/tests  # some currently fail due to missing matlab pulseq files
  requires:
    - pip
    - coverage >= 6.2
    - pytest

about:
  home: https://github.com/imr-framework/pypulseq
  summary: Pulseq in Python
  license: AGPL-3.0
  license_file: LICENSE

extra:
  recipe-maintainers:
    - AddYourGitHubIdHere

This can be built with conda build -c conda-forge . in the directory where you put this meta.yaml file.

This builds on my macbook (arm), but there are errors and warnings from unit tests (annotated above).

All of this should obviously go together with a CI pipeline and more unit tests, including getting the matlab pulseq files for the current test suite into the repository (see #144).

gabuzi avatar Nov 17 '23 18:11 gabuzi

~~Hi @gabuzi, thank you for the suggestion. Would you be willing to turn the yaml into a PR, so 1) you would get the credit if it gets merged, 2) it is easier to test and discuss that way?~~

btasdelen avatar Nov 17 '23 19:11 btasdelen

@btasdelen This wouldn't exist in this repo but in a conda-forge feedstock repository. You establish this by creating a PR in the conda-forge staged recipe repo. As such a PR doesn't really make sense for this.

wtclarke avatar Nov 17 '23 19:11 wtclarke

Oh, I get it now. Sorry for my confusion!

btasdelen avatar Nov 17 '23 20:11 btasdelen

@btasdelen Indeed, as @wtclarke mentioned, we should put it probably in a conda-forge feedstock.

The alternative to build it in this repo as part of a CI pipeline that we will eventually implement, does exist, though, but it would mean that we will also have to publish to a dedicated conda channel.

In my experience, conda channels are a frequent cause of confusion (take for example sigpy that exists on conda-forge and the frankong channel, notably with different versions). So, it's probably better to limit to conda-forge for the moment.

The reason why I put all that here first is to park it while we discuss instead of just charging ahead :)

gabuzi avatar Nov 17 '23 20:11 gabuzi

Yes, unless you are a very large project, definitely stick with conda-forge. People basically use it as the default.

wtclarke avatar Nov 17 '23 20:11 wtclarke

Any updates on this? Should take care of it @gabuzi?

lrlunin avatar Oct 29 '24 10:10 lrlunin

I think it would be great if you could get this implemented @lrlunin

schuenke avatar Oct 29 '24 11:10 schuenke

@lrunin, thanks for the push. Unfortunately, I haven't had too much time to focus on pypulseq recently. If you want you can give this a go, thanks! Feel free to use the recipe above as a starting point! It might need some tweaks to account for recent updates and changes in the ecosystem.

gabuzi avatar Oct 29 '24 11:10 gabuzi

I prepared the recipe for the conda-forge: https://github.com/conda-forge/staged-recipes/pull/28052

The blocking issue is now the fact that the PyPi version of the pypulseq is a little bit outdated with the dev branch. In the current PyPi version 1.4.2 the sigpy library is required and not presented in conda-forge. As far as I concerned this is not required anymore.

So please publish a new release to PyPi. The conda-forge version would pull it from the PyPi and no additional changes in your repository are needed.

To pass the checks of the conda-forge distrbution all mainternes listed need to leave a message in the pull request. Namely: @sravan953, @btasdelen , @FrankZijlstra and @schuenke.

Best wishes

lrlunin avatar Nov 01 '24 13:11 lrlunin

Thanks for preparing this @lrlunin

I already create a draft release for version v1.4.2.post1 here: https://github.com/imr-framework/pypulseq/releases

I would at least wait for #200 to be merged before the release, but imo this can be done soon.

schuenke avatar Nov 01 '24 15:11 schuenke

Short reminder for @sravan953 @btasdelen and @FrankZijlstra to leave a comment here: https://github.com/conda-forge/staged-recipes/pull/28052

schuenke avatar Nov 20 '24 11:11 schuenke

@lrlunin Do you think this could be progressed now? I think https://anaconda.org/conda-forge/sigpy is now available. Is there anything you need a hand with?

wtclarke avatar Mar 05 '25 15:03 wtclarke

@lrlunin Do you think this could be progressed now? I think https://anaconda.org/conda-forge/sigpy is now available. Is there anything you need a hand with?

I believe the main issue were some required versions mismatching/absense of several libs on conda-forge. I still would need the comments of the maintainers in the conda-forge repo.

I'll take a look either this week or 2 weeks later.

lrlunin avatar Mar 05 '25 16:03 lrlunin

Thanks. Note the conda-forge maintainers don't need to be the same as the package maintainers. It helps, but if people aren;t responding you can just have a subset, or even add others to spread the work (I'd be happy being listed).

wtclarke avatar Mar 05 '25 16:03 wtclarke

And any chance you could click rerun on the actions so that we can get an up-to-date feeling of whether there are any remaining issues? The logs of the previous ones have expired.

wtclarke avatar Mar 05 '25 16:03 wtclarke

@lrlunin Any chance of an update?

wtclarke avatar Mar 17 '25 11:03 wtclarke