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

Try OCP 7.8.1 #1589

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from
Draft

Try OCP 7.8.1 #1589

wants to merge 17 commits into from

Conversation

adam-urbanczyk
Copy link
Member

@adam-urbanczyk adam-urbanczyk commented May 20, 2024

  • Fix hashing
  • Fix vis mypy issues -> it seems that vtk 9.3 introduced stubs that are not fully correct

@adam-urbanczyk adam-urbanczyk marked this pull request as draft May 20, 2024 14:44
@adam-urbanczyk adam-urbanczyk added this to the 2.6 milestone Dec 15, 2024
@adam-urbanczyk
Copy link
Member Author

CC @jmwright

@adam-urbanczyk
Copy link
Member Author

Note to self: I cannot reproduce the crash locally with a manually fixed version of OCP

adam-urbanczyk referenced this pull request in CadQuery/pywrap Jan 2, 2025
@bernhard-42
Copy link
Contributor

I have OCP 7.8.1 successfully finishing all cadquery and build123d tests: https://github.com/bernhard-42/repackage-ocp/actions/runs/12619194289

I had random crashes in the exports tests before and analysed them. It all boiled down to lines failing that used an iterator on some OCCT objects. The culprit is the definition __iter__ in register_template_NCollection_*. py::make_iterator doesn't seem to handle lifetime of C++ objects correctly.
So I think it is not about the OCP implementation of __iter__, but an internal pybind11 problem, a bit like issue 3776 of pybind11 (or the ones being referenced).
I tried to use the newest pybind11 - which actually works with OCP - but the seg faults persisted.

So I decided to bypass the issue by replacing the 2 iterator uses in cadquery and the 3 in build123d by "standard" OCCT loops (using __call__ instead of __iter__). My patch of the latest cadquery master is:

diff --git a/cadquery/occ_impl/exporters/dxf.py b/cadquery/occ_impl/exporters/dxf.py
index 7a71173..296c442 100644
--- a/cadquery/occ_impl/exporters/dxf.py
+++ b/cadquery/occ_impl/exporters/dxf.py
@@ -343,8 +343,20 @@ class DxfDocument:
         spline.Transform(adaptor.Trsf())
 
         order = spline.Degree() + 1
-        knots = list(spline.KnotSequence())
-        poles = [(p.X(), p.Y(), p.Z()) for p in spline.Poles()]
+        
+        # knots = list(spline.KnotSequence())
+        knots = []
+        for i in range(1, spline.NbKnots() + 1):
+            knot = spline.Knot(i)
+            for j in range(spline.Multiplicity(i)):
+                knots.append(knot)
+
+        # poles = [(p.X(), p.Y(), p.Z()) for p in spline.Poles()]
+        poles = []
+        for i in range(1, spline.NbPoles() + 1):
+            p = spline.Pole(i)
+            poles.append((p.X(), p.Y(), p.Z()))
+
         weights = (
             [spline.Weight(i) for i in range(1, spline.NbPoles() + 1)]
             if spline.IsRational()
diff --git a/cadquery/occ_impl/shapes.py b/cadquery/occ_impl/shapes.py
index cedc480..dfa75bd 100644
--- a/cadquery/occ_impl/shapes.py
+++ b/cadquery/occ_impl/shapes.py
@@ -596,7 +596,7 @@ class Shape(object):
         Returns a hashed value denoting this shape. It is computed from the
         TShape and the Location. The Orientation is not used.
         """
-        return self.wrapped.HashCode(HASH_CODE_MAX)
+        return hash(self.wrapped)
 
     def isNull(self) -> bool:
         """
diff --git a/setup.py b/setup.py
index 9ad7b6b..16c700c 100644
--- a/setup.py
+++ b/setup.py
@@ -26,7 +26,7 @@ is_conda = "CONDA_PREFIX" in os.environ
 # Only include the installation dependencies if we are not running on RTD or AppVeyor or in a conda env
 if not is_rtd and not is_appveyor and not is_azure and not is_conda:
     reqs = [
-        "cadquery-ocp>=7.7.0,<7.8",
+        "cadquery-ocp>=7.8.0,<7.9",
         "ezdxf",
         "multimethod>=1.11,<2.0",
         "nlopt>=2.9.0,<3.0",

Pretty straightforward:

  • Replace the two iterators (__iter__) with standard loops (__call__).
  • Change hash behaviour
  • Allow OCP 7.8.1

This patch applied to cadquery master allowed all tests on all 4 platforms to run successfully, see for example https://github.com/bernhard-42/repackage-ocp/actions/runs/12619194289/job/35163428655

The patch might not be the solution (having the iterator would be nice), but I thought it could be helpful to share my analysis and results to help progressing this issue.

@bernhard-42
Copy link
Contributor

btw. for my builds I checkout commit 6cbeb64e9 of pywrap to avoid the regression I mentioned the other day

@adam-urbanczyk
Copy link
Member Author

Thanks, I think I have a manual solution for __iter__ but I need to figure out how to apply it with pywrap (or how to change pywrap to be able to apply it).

@CadQuery CadQuery locked and limited conversation to collaborators Jan 5, 2025
@adam-urbanczyk
Copy link
Member Author

adam-urbanczyk commented Jan 5, 2025

Note to self: CI here should fail due to different reason - when using the pyd file from azure locally I get a different error.

@adam-urbanczyk
Copy link
Member Author

adam-urbanczyk commented Jan 7, 2025

Added reference_internal only to TColStd_/TColgp_ returns and things seem to work. Mypy failure is due to small vtk api changes.

@CadQuery CadQuery unlocked this conversation Jan 7, 2025
@adam-urbanczyk
Copy link
Member Author

@bernhard-42 @jmwright things mostly work now. If you are still interested, you could start testing.

@bernhard-42
Copy link
Contributor

I tried my pipelines on github runners:

  • For Intel Mac, arm64 Mac and Ubuntu with Python 3.10-3.12 all tests (cadquery and build123d) were successful.
  • Windows is successful for Python 3.11 (for the other Python versions my pipeline is broken - independent of OCP)
  • For Python 3.13, again, something in my pipeline is wrong (independent of OCP)

So, looks good up to now, but a few test systems are missing due to broken pipelines.

@bernhard-42
Copy link
Contributor

fyi, my Pipeline now works.
I can build OCP 7.8.1.0 wheels for

  • Python 3.10, 3.11, 3.12, 3.13
    (for 3.13 I create my own cadquery_vtk package since vtk==9.3.1 is not on pypi for Python 3.13)
  • MacOS Intel (macos-13), MacOS arm64 (macos-14), Linux (ubuntu-20.04) and Windows (windows-2019)
  • with vtk (as cadquery_ocp) and without vtk (as cadquery_ocp_novtk)

For build123d all tests ran successfully for cadquery_ocp and cadquery_ocp_novtk (after patching VTK part out of it)

For cadquery all tests ran successfully for cadquery_ocp. However:

  • nlopt 2.9.0 does not exist for MacOS Intel on pypi => my tests install it from conda in this case
  • nlopt 2.9.0 and casadi both taken from pypi lead to a seg fault on Windows => my tests install it from conda in this case

pytest run:

          git clone https://github.com/cadquery/cadquery.git
          cd cadquery
          git checkout 23af517a66928e7c0bc3ebb6b86edf643af4751c

          patch -p1 < ../patches/cadquery.patch

          micromamba activate test
          pip install multimethod

          # workaround for nlopt 2.9.0 not available for Intel Mac
          if [[ "${{ matrix.os }}" == "macos-13" ]]; then
            micromamba install -y nlopt=2.9
          fi

          # workaround for pypi opt and casadi throwing a seg fault on exit 
          if [[ "$RUNNER_OS" == "Windows" ]]; then
            micromamba install -y nlopt casadi
          fi

          CONDA_PREFIX_BAK=$CONDA_PREFIX
          unset CONDA_PREFIX
          pip install .
          CONDA_PREFIX=$CONDA_PREFIX_BAK

          pytest tests -v -W ignore

Note, I check out one of the lastest cadquery commits just to have a stable base. This will be changed to master when the official OCP 7.8.1 is out

cadquery.patch:

diff --git a/cadquery/occ_impl/shapes.py b/cadquery/occ_impl/shapes.py
index cedc480..dfa75bd 100644
--- a/cadquery/occ_impl/shapes.py
+++ b/cadquery/occ_impl/shapes.py
@@ -596,7 +596,7 @@ class Shape(object):
         Returns a hashed value denoting this shape. It is computed from the
         TShape and the Location. The Orientation is not used.
         """
-        return self.wrapped.HashCode(HASH_CODE_MAX)
+        return hash(self.wrapped)
 
     def isNull(self) -> bool:
         """
diff --git a/setup.py b/setup.py
index 9ad7b6b..bd15298 100644
--- a/setup.py
+++ b/setup.py
@@ -24,9 +24,9 @@ is_azure = "CONDA_PY" in os.environ
 is_conda = "CONDA_PREFIX" in os.environ
 
 # Only include the installation dependencies if we are not running on RTD or AppVeyor or in a conda env
-if not is_rtd and not is_appveyor and not is_azure and not is_conda:
+if not is_rtd and not is_appveyor and not is_azure:
     reqs = [
-        "cadquery-ocp>=7.7.0,<7.8",
+        "cadquery-ocp>=7.8.0,<7.9",
         "ezdxf",
         "multimethod>=1.11,<2.0",
         "nlopt>=2.9.0,<3.0",
diff --git a/tests/test_assembly.py b/tests/test_assembly.py
index bae54ab..1093ffb 100644
--- a/tests/test_assembly.py
+++ b/tests/test_assembly.py
@@ -702,20 +702,17 @@ def test_save(extension, args, nested_assy, nested_assy_sphere):
         ("stl", ("STL",), {}),
     ],
 )
-def test_export(extension, args, kwargs, tmpdir, nested_assy):
+def test_export(extension, args, kwargs, nested_assy):
 
     filename = "nested." + extension
-
-    with tmpdir:
-        nested_assy.export(filename, *args, **kwargs)
-        assert os.path.exists(filename)
+    nested_assy.export(filename, *args, **kwargs)
+    assert os.path.exists(filename)
 
 
-def test_export_vtkjs(tmpdir, nested_assy):
+def test_export_vtkjs(nested_assy):
 
-    with tmpdir:
-        nested_assy.export("nested.vtkjs")
-        assert os.path.exists("nested.vtkjs.zip")
+    nested_assy.export("nested.vtkjs")
+    assert os.path.exists("nested.vtkjs.zip")
 
 
 def test_export_errors(nested_assy):

Note, pytest under Python 3.13 does not support context managers any more for the fixture temp_path, hence I patched the two test routines, since they don't seem to need the temp dir

I will copy over the build workflow into @jmwright 's òcp_build-system, and as soon as you have officially published 7.8.1, I will change the PYWRAP parameter from my pipeline to false and download the source from your repro during build instead of using pywrap.

The result will then be published to pypi as the OCP wheels.

@bernhard-42
Copy link
Contributor

This workaround

          CONDA_PREFIX_BAK=$CONDA_PREFIX
          unset CONDA_PREFIX
          pip install .
          CONDA_PREFIX=$CONDA_PREFIX_BAK

is really ugly. Is there any chance to allow using pip install of cadquery in a conda environment?

Use case: I use micromamba to create the base Python env, and afterwards I only use pip. I don't want to switch to pyenv or the likes, because sometimes I need conda packages and micromamba is the best compromise

@bernhard-42
Copy link
Contributor

The working pipeline can be found at https://github.com/bernhard-42/repackage-ocp/actions/runs/12717883699

Copy link

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.33%. Comparing base (23af517) to head (6207ab7).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1589      +/-   ##
==========================================
- Coverage   95.33%   95.33%   -0.01%     
==========================================
  Files          28       28              
  Lines        6842     6841       -1     
  Branches     1025     1025              
==========================================
- Hits         6523     6522       -1     
  Misses        194      194              
  Partials      125      125              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adam-urbanczyk
Copy link
Member Author

Mac passes, releasing OCP.

@adam-urbanczyk
Copy link
Member Author

This workaround

          CONDA_PREFIX_BAK=$CONDA_PREFIX
          unset CONDA_PREFIX
          pip install .
          CONDA_PREFIX=$CONDA_PREFIX_BAK

is really ugly. Is there any chance to allow using pip install of cadquery in a conda environment?

Use case: I use micromamba to create the base Python env, and afterwards I only use pip. I don't want to switch to pyenv or the likes, because sometimes I need conda packages and micromamba is the best compromise

Not really, we could use a custom env var to save you two lines. But you can also invoke the command in a subshell:

(unset CONDA_PREFIX && pip install . )

which makes the point moot IMO.

@bernhard-42
Copy link
Contributor

Fair.

@adam-urbanczyk
Copy link
Member Author

Tests pass with conda-forge ocp

@jmwright
Copy link
Member

@adam-urbanczyk I cannot find any issues with this PR.

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.

3 participants