Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC 78: gdal-utils package #3260

Merged
merged 7 commits into from
Mar 27, 2021
Merged

Conversation

idanmiara
Copy link
Collaborator

@idanmiara idanmiara commented Dec 3, 2020

What does this PR do?

This RFC suggests to rename osgeo.utils (which were introduced in GDAL 3.2) into osgeo_utils. Essentially moving the utils outside of the osgeo package (which includes the GDAL core bindings), into a new pure-Python package gdal-utils. gdal-utils new distribution wheel will be available in pypi for smooth upgrading via pip install --upgrade gdal-utils.

For maximum backwards compatibility, The gdal distribution wheel would still include both packages: osgeo and osgeo_utils.

This PR includes a suggested implementation.

@rouault @hobu @rcoup

Formatted version

https://github.com/talos-gis/gdal/blob/Branch_rfc78_py_modules/gdal/doc/source/development/rfc/rfc78_gdal_utils_package.rst

Tasklist

  • ADD YOUR TASKS HERE
  • Add test case(s)
  • Add documentation
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

@idanmiara idanmiara changed the title rfc/rfc78_osgeo_utils.rst - RFC 78: osgeo_utils Python package split RFC 78: osgeo_utils Python package split Dec 3, 2020
@idanmiara
Copy link
Collaborator Author

Do we need this .t ? We do release bugfixes version of GDAL every 2 months. That's the .z number

Good point. 2 months is a good timeframe. I'll remove that.

@idanmiara
Copy link
Collaborator Author

@rouault does the pypi section make sense to you?
Could you please reserve osgeo and osgeo_utils in pypi as they are still available?

@rouault
Copy link
Member

rouault commented Dec 3, 2020

Shouldn't the pypi package be "gdal" (bindings only) and "gdal_utils" (that would have a dependency on gdal) ?
"pip install osgeo" or osgeo_utils is confusing. The osgeo part is just a namespacing artifact, and perhaps not a super good one. People want to use GDAL, not OSGeo.

We probably need to mention a backward compatiblity issue if the "gdal" pip project no longer ships the utils

@idanmiara
Copy link
Collaborator Author

idanmiara commented Dec 3, 2020

I'm wondering about the utils namespace should it be osgeo_utils or gdal_utils?
I think I'd go with osgeo_utils because:

  • it's a single char difference from osgeo.utils
  • the modules inside utils already have a gdal prefix so from osgeo_utils import gdal_calc makes sense.
  • it leaves room for from osgeo_utils import ogr_something, osr_something

Shouldn't the pypi package be "gdal" (bindings only) and "gdal_utils" (that would have a dependency on gdal) ?
"pip install osgeo" or osgeo_utils is confusing. The osgeo part is just a namespacing artifact, and perhaps not a super good one. People want to use GDAL, not OSGeo.

We probably need to mention a backward compatiblity issue if the "gdal" pip project no longer ships the utils

So as you said, one option would be:
pip install gdal would install just the osgeo package that includes the bindings;
pip install gdal_utils would install the new utils package and the bindings as being its dependency.
This would raise another backward compatibility issue as you noted.

Second option would be:
pip install osgeo (or gdal_bindings?) would install just the osgeo package that includes just the bindings;
pip install gdal would install the new utils package and the bindings as being its dependency.
And no further backward compatibility issues with this option, right?

A third, option would be:
pip install osgeo would install just the osgeo package that includes just the bindings;
pip install osgeo_utils would install the new utils package and the bindings as being its dependency.
pip install gdal so that gdal would be considered as an empty meta-package name that depends on osgeo, osgeo_utils and perhaps a future osgeo_new_something if we find necessary.

I think the third way is the most consistent with the namespace names and also backwards compatible and maybe more future proof.

@idanmiara idanmiara force-pushed the Branch_rfc78_py_modules branch 4 times, most recently from 6f338b3 to 3f63bda Compare December 8, 2020 10:22
@idanmiara idanmiara force-pushed the Branch_rfc78_py_modules branch 2 times, most recently from e110c0b to 926cc26 Compare December 13, 2020 11:52
@idanmiara idanmiara changed the title RFC 78: osgeo_utils Python package split RFC 78: Python utils package Dec 13, 2020
@idanmiara
Copy link
Collaborator Author

idanmiara commented Dec 13, 2020

I've updated the RFC and released a test version of gdal_utils to pypi.

I mistakenly used version 3.3.0 on pypi so we cannot use this version for the release, we could use 3.3.0.0
I couldn't figure out why the Project description shows as raw although I specified the format in the setup.py
Do you prefer I'll remove gdal-utils from pypi and put it only on test.pypi till it is approved?

Open issues:

  1. @rouault @hobu - Would you like to share more comments? Would you like to discuss this RFC further before voting?
  2. Who will be the maintainer of this package? @hobu, as you are the maintainer of the gdal package, would you like to be listed as the maintainer of gdal-utils? or myself? or both of us?
  3. package naming (from gdal_utils import gdal_calc, ogr_foo, osr_bar vs from osgeo_utils import gdal_calc, ogr_foo, osr_bar)

@rouault
Copy link
Member

rouault commented Dec 22, 2020

@idanmiara I'm still lacking enthusiasm for this RFC, but maybe others will be or have suggestions to make it evolve. If you still want to pursue it, the process would be first to advertize it to gdal-dev for discussion before putting it to vote.

@idanmiara idanmiara changed the title RFC 78: Python utils package RFC 78: gdal-utils package Dec 22, 2020
@idanmiara
Copy link
Collaborator Author

@idanmiara I'm still lacking enthusiasm for this RFC, but maybe others will be or have suggestions to make it evolve. If you still want to pursue it, the process would be first to advertize it to gdal-dev for discussion before putting it to vote.

Done, thanks!

@idanmiara idanmiara force-pushed the Branch_rfc78_py_modules branch 4 times, most recently from 9c8ced1 to 47213e6 Compare January 16, 2021 17:39
@idanmiara
Copy link
Collaborator Author

idanmiara commented Jan 17, 2021

I have no idea what causes all these failures on the MacOS build.
Could @rouault or someone else drop me a hint please?

Edit: it seems to have been a flake, as it was solved by itself after another unrelated commit.

@idanmiara idanmiara force-pushed the Branch_rfc78_py_modules branch 2 times, most recently from 0795bd0 to b3d08d3 Compare January 20, 2021 20:42
@idanmiara
Copy link
Collaborator Author

Hi @rouault,
Would you please advise what's the next step regarding this PR?

@rouault
Copy link
Member

rouault commented Mar 15, 2021

Would you please advise what's the next step regarding this PR?

if you want still to have it adopted, you'll need to make a motion for it on the mailing list. I'm still unconvinced though

@idanmiara
Copy link
Collaborator Author

Would you please advise what's the next step regarding this PR?

if you want still to have it adopted, you'll need to make a motion for it on the mailing list. I'm still unconvinced though

@rouault I've revised the text of this RFC today to emphasize the main ideas and motivations. please read it again before you vote. Thanks!

@idanmiara idanmiara force-pushed the Branch_rfc78_py_modules branch 2 times, most recently from 8236e7f to 4ea1057 Compare March 26, 2021 12:29
@rouault
Copy link
Member

rouault commented Mar 26, 2021

Could you possibly rework a bit the history in a commit with the RFC text and another one with the implementation (for the implementation, potentially split up in bisect-able pieces if that makes sense) ?

@idanmiara
Copy link
Collaborator Author

Could you possibly rework a bit the history in a commit with the RFC text and another one with the implementation (for the implementation, potentially split up in bisect-able pieces if that makes sense) ?

done.

Do you think that ./gdal/swig/python/gdal-utils is the proper path for the sources? other options I had in mind:
./gdal/gdal-utils - as it's not really swig related.
./gdal-utils - maybe a better place if we think to split autotest sometime in the future?
I really don't have a solid preference...

@rouault
Copy link
Member

rouault commented Mar 26, 2021

done.

except that the commits where you change the osgeo/utils to osgeo_utils, and then the change in the imports should be done in one step, otherwise this breaks bisectability (by bysectability, I mean that ideally we should be able to checkout at a random master commit and have a fully working tree)

other options I had in mind:

The downside with your alternate proposals is that it is no longer obvious that gdal-utils is python related.

@idanmiara
Copy link
Collaborator Author

except that the commits where you change the osgeo/utils to osgeo_utils, and then the change in the imports should be done in one step, otherwise this breaks bisectability (by bysectability, I mean that ideally we should be able to checkout at a random master commit and have a fully working tree)

The logic behind the individual commits was that it's easier to track the changes I've made in each commit.
But I see what you mean, so I squashed the 4 following commits to make it bisactable:

  • osgeo.utils -> osgeo_utils (search replace)
  • move gdal/swig/python/osgeo/utils -> gdal/swig/python/gdal-utils/osgeo_utils
  • gdal/swig/python/setup.py - update paths
  • test_py_scripts.py - update osgeo_utils path

gdal.py - improved deprecation_warn
gdal/swig/python/scripts/*.py - remove redundant explicit parameter 'utils'
…cripts.py + update gdal setup.py

test_py_scripts.py - update osgeo_utils path
gdal/swig/python/setup.py - update paths
move gdal/swig/python/osgeo/utils -> gdal/swig/python/gdal-utils/osgeo_utils
osgeo.utils -> osgeo_utils (search-replace)
@rouault rouault added this to the 3.3.0 milestone Mar 27, 2021
@rouault rouault merged commit 8ca17c3 into OSGeo:master Mar 27, 2021
@idanmiara
Copy link
Collaborator Author

Thanks!
@rouault I've sent you an invitation to be an owner of https://pypi.org/project/gdal-utils/.

Please note that I've released a gdal-utils version 3.3.0 in the past by mistake, so for the first official release could be 3.3.0.X or 3.3.1.
Before the release of gdal 3.3.0 I'll yank all previous versions so only official versions would be available.

I would like to keep a "beta channel" for gdal-utils as a playground to test new features before they release officially.
As I don't want users to mistakenly think these as official versions I think I'll better use a different name, maybe gdal-utils-beta (unless you would like to suggest a different name).

@rouault
Copy link
Member

rouault commented Mar 27, 2021

I've released a gdal-utils version 3.3.0 in the past by mistake,

can't the release by removed / the project removed & recreated ?

@idanmiara
Copy link
Collaborator Author

I've released a gdal-utils version 3.3.0 in the past by mistake,

can't the release by removed / the project removed & recreated ?

Nope...
pypa/packaging-problems#74
I'll delete 3.3.0 so it wouldn't be possible to install it even by specifying gdal-utils == 3.3.0

@idanmiara idanmiara deleted the Branch_rfc78_py_modules branch April 18, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants