-
Notifications
You must be signed in to change notification settings - Fork 117
Mark tests that require network or geotiff dependency. #698
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
Open
sebastic
wants to merge
1
commit into
geopython:main
Choose a base branch
from
sebastic:offline-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply add the
geotifflibrary to the test requirements?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the library is not packaged, as mentioned in the OP:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean.
PyPI: https://pypi.org/project/geotiff/
Conda: https://github.com/conda-forge/python-geotiff-feedstock
geotiffis a core dependency as well:pywps/requirements.txt
Line 4 in 04f734b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no Debian package for the geotiff Python library:
See also: https://lists.debian.org/debian-gis/2025/12/msg00011.html
It is not imported unconditionally:
https://github.com/geopython/pywps/blob/main/pywps/validator/complexvalidator.py#L332-L338
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is specifically a Debian issue! My bad.
If we want to have this level of change, I would suggest that we remove
geotifffrom the core dependencies and add it to anextrasfor tracking optional dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastic @Zeitsperre @ldesousa can we limit support to conda-forge? I'm not sure if anyone is using direct installation on debian based dists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If core dependencies are adjusted to remove format-specific libraries, that will help reduce the package, which is great since not every deployment employs all combinations of data formats for their processing.
However, if this change is done, it should be marked as a higher release than a simple patch, since deployments relying on them could break.
Another thing to clarify in the release notes would be that these libraries are actually required only if using ">=STRICT" validation mode (they are imported under them in https://github.com/geopython/pywps/blob/main/pywps/validator/complexvalidator.py), so the formats could still be employed in I/O definitions with lower validation if the libraries are not installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSGeo-Live is using the Debian package currently:
https://github.com/OSGeo/OSGeoLive/blob/master/bin/install_pywps.sh#L53
That can of course be changed, I can then stop spending time on the pywps package in Debian.
CC: @kalxas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zarrv3 support has been notoriously difficult to adopt due to some very breaking changes. Most projects I work on are pinned below that version.geotiffwould need to address this issue before anything.I've worked on
geotiffas well and could try my hand at packaging it for Debian (for personal learning purposes). There are other packages (non-Python) I work on that I'd love to see in Debian/Ubuntu.I can justify putting some work towards this for this week. Fun end of year project, maybe. @sebastic Would you be fine with me cherry-picking your commit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're very welcome to join the Debian GIS team:
https://debian-gis-team.pages.debian.net/policy/index.html#membership
Sure, go ahead.