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

Add support for the Shapely extension #17

Closed
wants to merge 1 commit into from

Conversation

andreittr
Copy link
Contributor

@andreittr andreittr commented Jul 25, 2023

This change set adds support for the shapely extension in the main Python build.

Depends on Python 3.10 and Numpy:

and all their dependencies. Contains all commits from #16.
Commit c26d30d adds shapely support. (Will rebase once numpy is mainlined)

The Shapely microlibrary is a separate PR:

@razvand razvand self-assigned this Aug 5, 2023
@razvand razvand added the enhancement New feature or request label Aug 5, 2023
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I think we should add the other library dependencies to the Config.uk file (maybe as depends on statements or something), like the intel-intrinsis, libcxx* etc.

Comment on lines +90 to +92
if LIBPYTHON_SHAPELY
config LIBPYTHON3_EXTENSION_SHAPELY
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just select LIBPYTHON_STAPELY if the shapely extension is selected, or do a depends on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for numpy, this is the pattern for extensions and shapely isn't special enough to break it.
As for using depends on, in Kconfig a block like

if CONDITION
   config ...
endif

is by definition equivalent to

config ...
depends on CONDITION

so they are functionally identical, but there is an argument to be made for style.

We could change the entire file around to use this format --- it might save a handful of lines --- but it's something that should be done in a python-wide PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the entire file around to use this format --- it might save a handful of lines --- but it's something that should be done in a python-wide PR.

Agree, let's leave it like it is now.

@andreittr
Copy link
Contributor Author

Looks good, but I think we should add the other library dependencies to the Config.uk file (maybe as depends on statements or something), like the intel-intrinsis, libcxx* etc.

I strongly disagree. libcxx, intel-intrinsics, etc. are not dependencies of python's bindings to shapely, but of shapely itself (actually, of shapely's dependencies) and should thus logically be expressed in the latter library --- and more generally I'm of the opinion that only direct dependencies should be expressed through Kconfig, with indirect depends and selects being pulled in as required.
That way we have one authoritative source of truth for dependencies, one that we can update in a single, well-defined location when they change.

@StefanJum
Copy link
Member

I strongly disagree. libcxx, intel-intrinsics, etc. are not dependencies of python's bindings to shapely, but of shapely itself (actually, of shapely's dependencies) and should thus logically be expressed in the latter library --- and more generally I'm of the opinion that only direct dependencies should be expressed through Kconfig, with indirect depends and selects being pulled in as required. That way we have one authoritative source of truth for dependencies, one that we can update in a single, well-defined location when they change.

True. The final point is that everything should be eventually selected by some Config.uk. For example, is intel-intrinsics stated as a dependency by any library that ends up being selected when using python-shapely (I see that lib-python-shapely and lib-geos do not select it)?

@andreittr
Copy link
Contributor Author

True. The final point is that everything should be eventually selected by some Config.uk. For example, is intel-intrinsics stated as a dependency by any library that ends up being selected when using python-shapely (I see that lib-python-shapely and lib-geos do not select it)?

intel-intrinsics is selected by numpy, since that lib is the only one that directly requires intrinsics (shapely does not directly use intrinsics in any way, nor does geos -- perfectly reflecting their Config.uk contents).
Same with C++: shapely does not directly use any C++, only numpy and geos do, and they indeed do select libcxx; if either drops C++ as a requirement in the future (numpy existed for many years without C++ so it's pefectly reasonable) it's clear where this change needs to be reflected.

@StefanJum
Copy link
Member

intel-intrinsics is selected by numpy, since that lib is the only one that directly requires intrinsics (shapely does not directly use intrinsics in any way, nor does geos -- perfectly reflecting their Config.uk contents). Same with C++: shapely does not directly use any C++, only numpy and geos do, and they indeed do select libcxx; if either drops C++ as a requirement in the future (numpy existed for many years without C++ so it's pefectly reasonable) it's clear where this change needs to be reflected.

Perfect, then it's all good.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good @andreittr, I'll add the tag after all numpy and libgeos dependencies are merged.

@StefanJum
Copy link
Member

@andreittr you can rebase this now, #15 is merged 🎉

@andreittr
Copy link
Contributor Author

andreittr commented Aug 11, 2023

@andreittr you can rebase this now, #15 is merged tada

Will rebase it directly onto numpy once it gets merged.

Done

This change only adds the necessary module registrations and Kconfig
options. Shapely itself is provided by its own unikraft port.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good with me now, good job!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved-by: Razvan Deaconescu razvand@unikraft.io

@andreittr andreittr deleted the ttr/py310-shapely branch August 14, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants