kikit: use latest shapely by imincik · Pull Request #268277 · NixOS/nixpkgs

imincik · 2023-11-18T11:05:47Z

Description of changes

Please consider to use latest shapely version (currently 2.0.2) instead of
obsolete, custom built version 1.8.4.

I am aware of upstream plans to move away from shapely (https://github.com/
yaqwsx/KiKit/issues/574) and doesn't want to support newer shapely versions,
all kikit tests are passing with the new one.

Things done

jfly · 2023-11-18T12:53:40Z

@imincik, thanks for the contribution!

I'm not sure how much to trust kikit's unit tests here. Shapely 2.0 is a big release that documents a number of breaking changes. I unfortunately don't know enough about kikit to know how to exercise any of its interactions with shapely. Are you a kikit user? Are you able to confirm that things still work after this change?

Perhaps most importantly, what's the purpose of this change? Is the old version of shapely causing a maintenance burden I'm not familiar with? Without more context, it doesn't seem worth the risk to me, especially as we expect all this to disappear someday when upstream drops shapely entirely.

imincik · 2023-11-18T14:49:25Z

I am not a kikit user, but I am one of the shapely package maintainers.

I don't think that keeping some random, not maintained copy of dependency (shapely 1.8.4 is not even the latest 1.8.x version) inside of a package is a good practice.

If you are really sure that we can't trust kikit's tests, maybe the better solution is to package shapely 1.8.5 as part of shapely package. Then you can just use it and it will be maintained together with the latest version. I can do that for you if you agree. In this way it can be used by others as well.

imincik · 2023-11-18T15:28:14Z

I just tried to run

./result/bin/kikit panelize \
    --layout 'grid; rows: 2; cols: 2; space: 2mm' \
    --tabs 'fixed; width: 3mm; vcount: 2' \
    --cuts 'mousebites; drill: 0.5mm; spacing: 1mm; offset: 0.2mm; prolong: 0.5mm' \
    --framing 'frame; width: 5mm; space: 3mm; mintotalheight: 100mm; mintotalwidth: 100mm' \
    --tooling '3hole; hoffset: 2.5mm; voffset: 2.5mm; size: 1.5mm' \
    --fiducials '3fid; hoffset: 5mm; voffset: 2.5mm; coppersize: 2mm; opening: 1mm;' \
    --text 'simple; text: yaqwsxs panel with minimal dimensions; anchor: mt; voffset: 2.5mm; hjustify: center; vjustify: center;' \
    --post 'millradius: 1mm' \
   kikit/code/docs/resources/conn.kicad_pcb panel.kicad_pcb

and it didn't complain.

I suggest to implement nix package tests using commands like this. If we are not sure about upstream tests quality, then nix package test make a sense even more.

jfly · 2023-11-20T05:34:29Z

I just tried to run [...] and it didn't complain.

That's promising! (And I agree, we should do some investigation into how comprehensive upstream tests are, and if they're not, we should either contribute tests upstream, or add tests to our derivation.)

That said, I'm afraid I still get can't comfortable with upgrading to shapely 2.0+ without understanding the reason for this commit. I can't tell if this upper bound was added out of an abundance of caution, or because something legitimately broke. Furthermore, I wouldn't really want to bother upstream with questions like this, as they clearly aren't interested in spending any time on shapely.

maybe the better solution is to package shapely 1.8.5 as part of shapely package

I prefer this. In fact, I'm not even sure why we packaged 1.8.4 instead of 1.8.5. Perhaps because 1.8.5 was never packaged by nixpkgs (it looks like we jumped from 1.8.4 to 2.0.0 here). Perhaps @matusf remembers?

matusf · 2023-11-20T23:52:43Z

Hi, sorry for the late response. I don't feel confident about upgrading a major version of a dependency, especially when the upstream project has constraints on the version. I also don't think we can rely solely on the unit tests, particularly in a dynamic language like Python. This suggestion should instead be offered to the developers of Kikit (as @jfly mentioned, we've already done that, and they plan to stop using Shapely). Therefore, I suggest we stick with the 1.x.x version.

Regarding the tests, since I'm not a Kikit user, I don't feel confident in creating thorough tests for Kikit. I also believe they should be maintained by the upstream project rather than here. For instance, a new version of Kikit might break the tests, but that could be the desired behavior. It would be challenging to differentiate intentional changes in behavior from potential bugs.

Maybe @matusf remembers?

Yes, as you mentioned, version 1.8.5 was never packaged by nixpkgs. We selected the latest version in history that satisfied the constraints.

Perhaps a better solution is to include Shapely 1.8.5 as part of the Shapely package?

Yes, if we decide to upgrade the version of Shapely, I'd prefer to move from 1.8.4 to the 1.8.5 (latest before v2).

imincik · 2023-11-22T11:38:02Z

I understand what you are saying.

I was just about to create frozen shapely_1_8 package for you, but unfortunately, I realised that it is going to have a problems with latest geos which is about to be merged soon.

I just tried to build kikit's copy of Shapely with latest GEOS PR:

$ gh pr checkout 268130

$ nix-build -I nixpkgs=. --expr '(import <nixpkgs> {}).pkgs.python3.pkgs.callPackage ./pkgs/by-name/ki/kikit/shapely {}'

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[MULTIPOINT (1 1 1, 2 2)-MULTIPOINT Z (1 1 1, 2 2 0)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[MULTIPOINT (1 1, 2 2 2)-MULTIPOINT Z (1 1 0, 2 2 2)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[LINESTRING (1 1 1, 2 2)-LINESTRING Z (1 1 1, 2 2 0)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[POLYGON ((0 0 0, 1 0 0, 1 1, 0 1 0, 0 0 0))-POLYGON Z ((0 0 0, 1 0 0, 1 1 0, 0 1 0, 0 0 0))] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[LINESTRING (1 1, 2 2 2)-LINESTRING (1 1, 2 2)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[POLYGON ((0 0, 1 0 1, 1 1, 0 1, 0 0))-POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
FAILED tests/test_parallel_offset.py::OperationsTestCase::test_parallel_offset_linestring - AssertionError: <shapely.geometry.linestring.LineString object at 0x7fff729...
FAILED tests/test_voronoi_diagram.py::test_edges - assert False
======= 8 failed, 475 passed, 1 skipped, 2 xfailed, 13 warnings in 1.19s =======
/nix/store/wr08yanv2bjrphhi5aai12hf2qz5kvic-stdenv-linux/setup: line 1559: pop_var_context: head of shell_variables not a function context
error: builder for '/nix/store/m0fgbibc3jvsv22ac8inc98vfdq5v3ar-python3.11-Shapely-1.8.4.drv' failed with exit code 1;
       last 10 log lines:
       > FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[MULTIPOINT (1 1 1, 2 2)-MULTIPOINT Z (1 1 1, 2 2 0)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
       > FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[MULTIPOINT (1 1, 2 2 2)-MULTIPOINT Z (1 1 0, 2 2 2)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
       > FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[LINESTRING (1 1 1, 2 2)-LINESTRING Z (1 1 1, 2 2 0)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
       > FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[POLYGON ((0 0 0, 1 0 0, 1 1, 0 1 0, 0 0 0))-POLYGON Z ((0 0 0, 1 0 0, 1 1 0, 0 1 0, 0 0 0))] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
       > FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[LINESTRING (1 1, 2 2 2)-LINESTRING (1 1, 2 2)] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
       > FAILED tests/test_create_inconsistent_dimensionality.py::test_create_from_wkt[POLYGON ((0 0, 1 0 1, 1 1, 0 1, 0 0))-POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))] - shapely.errors.WKTReadingError: Could not create geometry because of errors...
       > FAILED tests/test_parallel_offset.py::OperationsTestCase::test_parallel_offset_linestring - AssertionError: <shapely.geometry.linestring.LineString object at 0x7fff729...
       > FAILED tests/test_voronoi_diagram.py::test_edges - assert False
       > ======= 8 failed, 475 passed, 1 skipped, 2 xfailed, 13 warnings in 1.19s =======
       > /nix/store/wr08yanv2bjrphhi5aai12hf2qz5kvic-stdenv-linux/setup: line 1559: pop_var_context: head of shell_variables not a function context

It means that if we want to keep kikit on functional shapely version we need to keep shapely 1.8.x along with GEOS 3.11.x .

jfly · 2023-11-22T19:55:06Z

Ah, darn. Now I'm starting to see how keeping this old version of shapely can evolve into a maintenence headache for other stuff in nixpkgs.

Let me know if you'd like some help introducing a pinned geos as well. I also think that if this turns into a third thing we need to pin just to keep kikit happy, I could see us pushing back upstream and saying this package is at risk of getting dropped/being marked broken in nixpkgs. Maybe that'll encourage them to upgrade geometry engines... 🤞

imincik · 2023-12-06T16:02:10Z

@jfly @matusf , I packaged GEOS 3.11 and shapely 1.8 and changed kikit to use it in #268130 . Please review.

imincik · 2023-12-06T16:03:09Z

Shapely is solved as part of #268130 .

kikit: use latest shapely

bf99bd7

ofborg bot requested review from jfly and matusf November 18, 2023 12:22

ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 18, 2023

imincik mentioned this pull request Dec 6, 2023

geos: 3.11.2 -> 3.12.1 #268130

Merged

13 tasks

imincik closed this Dec 6, 2023

jfly mentioned this pull request Aug 20, 2024

python3Packages.shapely_1_8: disable failing test #331285

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

kikit: use latest shapely#268277

kikit: use latest shapely#268277
imincik wants to merge 1 commit intoNixOS:masterfrom
imincik:kikit-use-latest-shapely

imincik commented Nov 18, 2023

Uh oh!

jfly commented Nov 18, 2023 •

edited

Loading

Uh oh!

imincik commented Nov 18, 2023 •

edited

Loading

Uh oh!

imincik commented Nov 18, 2023 •

edited

Loading

Uh oh!

jfly commented Nov 20, 2023

Uh oh!

matusf commented Nov 20, 2023

Uh oh!

imincik commented Nov 22, 2023 •

edited

Loading

Uh oh!

jfly commented Nov 22, 2023

Uh oh!

imincik commented Dec 6, 2023

Uh oh!

imincik commented Dec 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

imincik commented Nov 18, 2023

Description of changes

Things done

Uh oh!

jfly commented Nov 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imincik commented Nov 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imincik commented Nov 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfly commented Nov 20, 2023

Uh oh!

matusf commented Nov 20, 2023

Uh oh!

imincik commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfly commented Nov 22, 2023

Uh oh!

imincik commented Dec 6, 2023

Uh oh!

imincik commented Dec 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jfly commented Nov 18, 2023 •

edited

Loading

imincik commented Nov 18, 2023 •

edited

Loading

imincik commented Nov 18, 2023 •

edited

Loading

imincik commented Nov 22, 2023 •

edited

Loading