search

imr-framework / pypulseq

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

Fix direct install from GitHub repo #202

Closed schuenke closed 2 months ago

schuenke commented 3 months ago

With #195 we switched from setup.py to pyproject.toml, where we defined:

[tool.setuptools.packages.find]
where = ["pypulseq"]

This works fine when you install pypulseq from a local folder using pip install ., but not if you want to install it directly from GitHub using pip install git+https://github.com/imr-framework/pypulseq.

With the direct install from GitHub, no pypulseq package will be created, but only the modules in our pypulseq folder (like SAR, seq_examples, Sequence, ...) will be installed as packages.

For me, removing the 2 lines above solves the problem, which makes sense IMO, because this is meant to tell setuptools where to look for our package and not which folder IS our package. Setuptools detects the src-layout, where the pypulseq folder would be in a src folder (we should switch to this layout !!!) and the flat-layout (the one we are still using) automatically. This is why we don't have to specify where to look for our package at all.

FrankZijlstra commented 3 months ago

Not an expert on this, but looks fine. Surprising that the behaviour is different depending on the install options. Have you checked whether it affects packaging the pypi package?

schuenke commented 3 months ago

Not an expert on this, but looks fine. Surprising that the behaviour is different depending on the install options. Have you checked whether it affects packaging the pypi package?

I didn't test it on PyPi directly, but tried the build command locally and installed it from the generated wheel in a clean env. That worked. Therefore, I am pretty sure it will work with PyPi as well.

btasdelen commented 2 months ago

Yeah, sorry, apparently I misunderstood what it does, I thought it would add pypulseq as a package, too.

Would you like to convert to src hierarchy in this PR, or should we go ahead and merge?

FrankZijlstra commented 2 months ago

Moving everything to src might create big merge conflicts on the other PRs? I'd suggest to incorporate those first (and I have one more adding the sequence examples back to the sequence unit tests).

schuenke commented 2 months ago

I agree that we should merge this one as well as #190 and #200 first and then create a seperate PR for switching to the src-layout.