search

inogs / bit.sea

4 stars 3 forks source link

refactor and basic packaging #14

Closed jacopo-exact closed 2 months ago

jacopo-exact commented 2 months ago

move project code in subfolder and add pyproject and gitignore It is now possible to build a basic wheel for the bitsea package. For the time being, only the commons and utilities are exported.

cc @spiani

jacopo-exact commented 2 months ago

@spiani for ease of review, there is no actual code change

spiani commented 2 months ago

I strongly believe this is a good idea. However, several factors have prevented me from carrying out this refactoring in the past.

If we proceed with this refactoring, it makes sense that the bitsea packages should be imported as: import bitsea.commons.whatever rather than simply using commons.whatever. While this is desirable, it would impact nearly every script written in OGS over the past decade.

@gbolzon, I think that maybe this could be a good opportunity to take the leap. What do you think?

jacopo-exact commented 2 months ago

I strongly believe this is a good idea. However, several factors have prevented me from carrying out this refactoring in the past.

If we proceed with this refactoring, it makes sense that the bitsea packages should be imported as: import bitsea.commons.whatever rather than simply using commons.whatever. While this is desirable, it would impact nearly every script written in OGS over the past decade.

This is only partially true. If you just copy the code and .../bit.sea/bitsea/ to the PYTHONPATH, you'll have a compatibility layer for free. That is more or less like running pip install -e . from within that directory.

In short, there are only benefits and the backward compatibility is trivial.

spiani commented 2 months ago

I understand, but it's a bit more complicated. For instance, consider the commons.submask file in bit.sea (which needs to import the mask file). Should it use import commons.mask or import bitsea.commons.mask?

If we go with the first option (as it would be if we accept the current state of the pull request), then bitsea would no longer function as a proper package. This could cause issues when people try to import it after having installed it using pip, for example.

On the other hand, if we change the imports to the bitsea structure, simply adding the bitsea directory to the PYTHONPATH will not be sufficient, because Python will fail to resolve the imports made by the bitsea files.

Of course, you could add both .../bit.sea and .../bit.sea/bitsea to your PYTHONPATH, and this should, in theory, resolve all the import issues. However, in this case, Python will treat commons.mask and bitsea.commons.mask as two different modules, which could break certain mechanisms (such as equality checks between two Enum objects).

I believe we should take the burden and change the imports once for all. This is the correct long-term solution.

jacopo-exact commented 2 months ago

I would push the burden of compatibility on users. Each user must choose whether to use the bitsea package in legacy mode (add bit.sea/bitsea to pythonpath) or in the suggested way (they themselves create a pyproject and add bitsea as a dependency in a clean fashion). No change for legacy code, yet enabling new code to use modern constructs.

spiani commented 2 months ago

I spoke with @gbolzon, and we decided to update all imports by adding bitsea. in front of the package names. This will be the correct way to use bit.sea moving forward. Users can still take advantage of the "compatibility layer" by adding .../bit.sea/bitsea to their PYTHONPATH, but I believe this approach makes everything more consistent.

I opened a new pull request that includes all the changes introduced in this one and prepend the bitsea. to the internal imports.

jacopo-exact commented 2 months ago

ok, thanks. Mind that this PR is still WIP. In particular the dependencies are still to be defined. Happy to help either assisting or training.

spiani commented 2 months ago

Thank you very much for your help! We've updated the imports and merged the commits into the current master. It may have been a bit premature, but given the significant impact on the code, we decided to integrate it as soon as possible.

If you'd like to improve the packaging, feel free to rebase this branch and continue.

Let us know if you think we're heading in the right direction!

jacopo-exact commented 2 months ago

Let us know if you think we're heading in the right direction!

From my end, the goal of this PR is that I can use stuff from MITgcm_BFM_chain and MITgcm_BC_IC without assuming that I have the bitsea code in my path, but rather explicitly declaring bitsea as a dependency. I shall provide further patches as I discover issues with the packaging.