nipreps / synthstrip

Synthstrip integration to be used across nipreps
Apache License 2.0
6 stars 1 forks source link

Namespace packaging #1

Closed oesteban closed 2 months ago

oesteban commented 1 year ago

shouldn't this:

https://github.com/nipreps/synthstrip/blob/dcc89bee00275374daeeaf6007ba90a35a4bdbca/pyproject.toml#L6

be

 name = "nipreps-synthstrip"

as per https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages?

And then also (https://setuptools.pypa.io/en/latest/userguide/package_discovery.html#finding-namespace-packages):

[tool.setuptools.packages.find]
where = ["nipreps"]
mgxd commented 1 year ago

Yep, the name should absolutely be nipreps-synthstrip, that was a remnant of my earlier fiddling.

Not sure about adding a setuptools command though, hatchling should handle this automatically I believe (cc our resident hatchling expert @effigies)

oesteban commented 1 year ago

What about the other two submodules I proposed?

nipreps.interfaces nipreps.workflows

mgxd commented 1 year ago

I don't remember that proposal - are you suggesting the import should look like:

from nipreps.interfaces.synthstrip import SynthStrip
effigies commented 1 year ago

Not sure about adding a setuptools command though, hatchling should handle this automatically I believe

Yes, hatchling and setuptools will (per spec) ignore each other's config. I think you're right that this should work (I would just build and unpack the sdist and wheel to verify) as-is, but if you want to be explicit, you will want the package to be nipreps and you might use only-include = ["nipreps/synthstrip"]. See https://hatch.pypa.io/latest/config/build/#explicit-selection.

oesteban commented 1 year ago

I don't remember that proposal

Sorry, it's this - https://github.com/nipreps/nireports/pull/71#issue-1672572206

  • are you suggesting the import should look like:
from nipreps.interfaces.synthstrip import SynthStrip

I think so, for the interface. But to ease our lives we can put the nipype interface with the other interfaces, and leave this package even more lightweight.

effigies commented 1 year ago

I would be up for trying to keep the interface and wrapped utility in the same package. We could even add a pydra task while we're at it.

nipreps/
  synthstrip/
  tasks/
    synthstrip/
  interfaces/
    synthstrip/

That way adapting the interface/task can be done in lockstep with changes to the utility and we can use PyPI don't need to depend on any versioning machinery in nipype or pydra.

I would keep nipype and pydra as optional dependencies.

oesteban commented 1 year ago

This would be a good topic to discuss in the next IT - don't you think?

cc/ @esavary

mgxd commented 1 year ago

My inclination is on keeping the namespace as lightweight as possible, and IMO nipreps.<tools> does enough to distinguish 1) what tool it is and 2) it is maintained by this organization.

I would keep nipype and pydra as optional dependencies.

+1 on anytime we can make the installation smaller.

What we could then do is create a separate cookiecutter (nipreps-submodules?) that creates the expected layout (building off chris's)

nipreps/
  synthstrip/
    <code>
    ...
    ext/
        nipype.py
        pydra.py
effigies commented 1 year ago

Yeah, I'm fine with bundling it inside one namespace. I would just be explicit and call the submodule wrapper or wrappers:

from nipreps.<tool>.wrapper.nipype import <Interface>
from nipreps.<tool>.wrapper.pydra import <Task>
oesteban commented 2 months ago

@mgxd can we finalize this? Having a pypi package would allow me to drop the code from mriqc and start using it elsewhere.

mgxd commented 2 months ago

@oesteban done

https://pypi.org/project/nipreps-synthstrip/

oesteban commented 2 months ago

Thanks!