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

Updated setup.py with OCP being available on PyPi now #1085

Merged
merged 26 commits into from
May 23, 2022
Merged

Updated setup.py with OCP being available on PyPi now #1085

merged 26 commits into from
May 23, 2022

Conversation

jmwright
Copy link
Member

When I test locally this takes care of all dependencies, and gives users a couple options (development and IPython installs).

@dcowden Does this look good to you from your experience with setup.py for CQ 1.x?

@adam-urbanczyk Do you think not installing IPython by default will cause any problems? I've made IPython an optional install that the user has to specifically opt into. I did this because I believe that IPython is only used for jupyter_tools.py, which many users don't need. IPython brings its own set of dependencies which most users won't use.

@fpq473
Copy link
Contributor

fpq473 commented May 19, 2022

Minor suggestions:

  • Require nptyping>=2 instead of just nptyping
  • Specify minimum python version of 3.8 (and max of 3.10?)
  • Update repo url from dcowden/cadquery to CadQuery/cadquery

If we're staying with setup.py for now then fwiw this is the reference I found helpful: https://setuptools.pypa.io/en/latest/references/keywords.html

@roipoussiere may have given more thought to various metadata when he did his proof-of-concept.

@jmwright
Copy link
Member Author

@fpq473 Thanks for the suggestions.

If we're staying with setup.py for now...

There's a comment in #1054 that setup.py is still needed for CI, so I think we'll keep both this and project.toml in the repo.

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #1085 (09975de) into master (cee66ff) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

❗ Current head 09975de differs from pull request most recent head 7cff170. Consider uploading reports for the commit 7cff170 to get more accurate results

@@            Coverage Diff             @@
##           master    #1085      +/-   ##
==========================================
- Coverage   96.37%   96.35%   -0.02%     
==========================================
  Files          40       40              
  Lines        9496     9500       +4     
  Branches     1256     1256              
==========================================
+ Hits         9152     9154       +2     
- Misses        202      204       +2     
  Partials      142      142              
Impacted Files Coverage Δ
cadquery/__init__.py 87.50% <60.00%> (-12.50%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@fpq473
Copy link
Contributor

fpq473 commented May 19, 2022

One more thought - I tried the installation and it works seamlessly, but the version of cadquery is hardcoded to 2.1 instead of something reflecting the git revision. This will likely be quite confusing when users ask for help.

One fix may be to use setuptools_scm to set a meaningful version (this diff slightly pollutes cadquery namespace, but can be fixed):

diff --git a/cadquery/__init__.py b/cadquery/__init__.py
index 4645a3d..1faa7ad 100644
--- a/cadquery/__init__.py
+++ b/cadquery/__init__.py
@@ -1,3 +1,11 @@
+from importlib.metadata import version, PackageNotFoundError
+
+try:
+    __version__ = version("cadquery")
+except PackageNotFoundError:
+    # package is not installed
+    pass
+
 # these items point to the OCC implementation
 from .occ_impl.geom import Plane, BoundBox, Vector, Matrix, Location
 from .occ_impl.shapes import (
@@ -69,5 +77,3 @@ __all__ = [
     "plugins",
     "Sketch",
 ]
-
-__version__ = "2.1"
diff --git a/setup.py b/setup.py
index 4c0b5cd..7499b7c 100644
--- a/setup.py
+++ b/setup.py
@@ -14,11 +14,9 @@
 import os
 from setuptools import setup, find_packages
 
-version = "2.1"
-
 setup(
     name="cadquery",
-    version=version,
+    use_scm_version=True,
+    setup_requires=["setuptools_scm"],
     url="https://github.com/CadQuery/cadquery",
     license="Apache Public License 2.0",
     author="David Cowden",

which gives a version like this:

$ pip install -e ./cadquery
...
Successfully installed cadquery-2.2.dev423+g9454dc4
$ python -c 'import cadquery; print(cadquery.__version__)'
2.2.dev423+g9454dc4
$

I should note that #924 proposes using setuptools_scm to generate the same meaningful version for conda builds between releases.

@jmwright
Copy link
Member Author

jmwright commented May 19, 2022

...using setuptools_scm to generate the same meaningful version for conda builds between releases.

@fpq473 That does seem like it would be helpful.

I'm currently having trouble with the ReadTheDocs build. It seems like conda is trying to install cadquery-ocp from its repositories even though (if I'm looking at it correctly), that should be installed via pip. Maybe I just need to switch to a pip installation for ReadTheDocs, but I'm also concerned that I've introduced something that will cause conda build problems too.

@fpq473
Copy link
Contributor

fpq473 commented May 20, 2022

Is it possible that the ReadTheDocs builds are being run on an old distro like Ubuntu 18.04? That distro is not compatible with cadquery-ocp wheels, which have the manylinux_2_31 tag. Can we try specifying 22.04 via build.os in .readthedocs.yaml?

@jmwright
Copy link
Member Author

Can we try specifying 22.04 via build.os in .readthedocs.yaml?

That's a good thought. I'll try that.

@adam-urbanczyk
Copy link
Member

adam-urbanczyk commented May 20, 2022

@jmwright please do not use ocp-vtk in the existing pipelines. If you want to test it here, I'd suggest adding separate one. I'd actually prefer the CI of CQ to not depend on ocp-vtk.

Regarding IPython I think it is fine.

@adam-urbanczyk
Copy link
Member

Is it possible that the ReadTheDocs builds are being run on an old distro like Ubuntu 18.04? That distro is not compatible with cadquery-ocp wheels, which have the manylinux_2_31 tag. Can we try specifying 22.04 via build.os in .readthedocs.yaml?

What is the rationale behind changing the minimum version that OCP requires?

@jmwright
Copy link
Member Author

...please do not use ocp-vtk in the existing pipelines.

@adam-urbanczyk I actually don't want to use it in the pipelines, I'm just trying to make CadQuery installable via pip, including from the git repo. The challenge has been that the conda environment.yml file relies on setup.py so any changes to that effect CI. I guess I'll start down the path of project.toml to try to completely separate the wheel installs from the CI system. That may require adding an option to the environment.yml file to force the use setup.py instead of project.toml, but I'm not sure how that will work yet.

@jmwright
Copy link
Member Author

@adam-urbanczyk @lorenzncode I think this PR is also ready for review. It was a lot more difficult that I expected to get CI, PyPi and Anaconda to play well together, but I think everything is working as it should now. Thanks again to @fpq473 for continuing to give feedback and ideas.

Copy link
Member

@adam-urbanczyk adam-urbanczyk left a comment

Choose a reason for hiding this comment

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

LGTM, just one question on the version handling.

from importlib.metadata import version, PackageNotFoundError

try:
version__ = version("cadquery")
Copy link
Member

Choose a reason for hiding this comment

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

Does this work when installed via conda?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it may since it leverages importlib, but it's hard to be sure. I think the version variable in setup.py is being used by conda when building packages too, so I left it hardcoded at 2.1 because I was afraid of breaking something downstream. However, we could try using the scm version number there in addition to the PyPi packages if you want, which should give us traceability down to the commit level on conda packages. I think that would be nice to be able to ask users to give the full version number when reporting bugs.

That version line has a typo in it that @fpq473 pointed out which I'll fix and I'll also give the conda version number a try just to see what happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think I'm mistaken about the conda build using the version variable in setup.py. There was an error in an early build that made it look that way, but I think it was actually a mistake within my setup.py changes. I think the that the cq.version call should probably still work with conda, but it's hard to know for sure until we try it with the conda packages.

Copy link
Member

@lorenzncode lorenzncode left a comment

Choose a reason for hiding this comment

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

I tested the pip install. It worked for me. Here is my install log.

It looks like the hardcoded __version__ in __init__.py should be removed. Notice version is still reported as 2.1.

log $ uname -a Linux fedora 5.17.5-200.fc35.x86_64 #1 SMP PREEMPT Thu Apr 28 15:41:41 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

$ which python
/usr/bin/python

$ python --version
Python 3.10.4

$ python -m venv ~/tools/venv/py310

$ . ~/tools/venv/py310/bin/activate

$ which python
/home/lorenzn/tools/venv/py310/bin/python

$ which pip
/home/lorenzn/tools/venv/py310/bin/pip

$ pip install --upgrade pip
Requirement already satisfied: pip in /home/lorenzn/tools/venv/py310/lib/python3.10/site-packages (21.2.3)
Collecting pip
Using cached pip-22.1.1-py3-none-any.whl (2.1 MB)
Installing collected packages: pip
Attempting uninstall: pip
Found existing installation: pip 21.2.3
Uninstalling pip-21.2.3:
Successfully uninstalled pip-21.2.3
Successfully installed pip-22.1.1

$ pip install https://github.com/jmwright/ocp-build-system/releases/download/7.5.3/cadquery_ocp-7.5.3-cp310-cp310-manylinux_2_31_x86_64.whl
Collecting cadquery-ocp==7.5.3
Downloading https://github.com/jmwright/ocp-build-system/releases/download/7.5.3/cadquery_ocp-7.5.3-cp310-cp310-manylinux_2_31_x86_64.whl (165.6 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 165.6/165.6 MB 8.0 MB/s eta 0:00:00
Installing collected packages: cadquery-ocp
Successfully installed cadquery-ocp-7.5.3.0

$ pip install git+https://github.com/CadQuery/cadquery.git@setup
Collecting git+https://github.com/CadQuery/cadquery.git@setup
Cloning https://github.com/CadQuery/cadquery.git (to revision setup) to /tmp/pip-req-build-mezmsscm
Running command git clone --filter=blob:none --quiet https://github.com/CadQuery/cadquery.git /tmp/pip-req-build-mezmsscm
Running command git checkout -b setup --track origin/setup
Switched to a new branch 'setup'
branch 'setup' set up to track 'origin/setup'.
Resolved https://github.com/CadQuery/cadquery.git to commit 162e3c5
Preparing metadata (setup.py): started
Preparing metadata (setup.py): finished with status 'done'
Requirement already satisfied: cadquery-ocp in /home/lorenzn/tools/venv/py310/lib/python3.10/site-packages (from cadquery==2.2.dev443+g162e3c5) (7.5.3.0)
Collecting ezdxf
Using cached ezdxf-0.17.2-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (2.6 MB)
Collecting multimethod
Using cached multimethod-1.8-py3-none-any.whl (9.8 kB)
Collecting nlopt
Using cached nlopt-2.7.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (423 kB)
Collecting nptyping>=2
Using cached nptyping-2.0.1-py3-none-any.whl (19 kB)
Collecting typish
Using cached typish-1.9.3-py3-none-any.whl (45 kB)
Collecting casadi
Using cached casadi-3.5.6.pre2-cp310-none-manylinux_2_5_x86_64.whl (42.8 MB)
Collecting path
Using cached path-16.4.0-py3-none-any.whl (26 kB)
Collecting numpy>=1.20.0
Using cached numpy-1.22.4-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (16.8 MB)
Collecting typing-extensions
Using cached typing_extensions-4.2.0-py3-none-any.whl (24 kB)
Collecting pyparsing>=2.0.1
Using cached pyparsing-3.0.9-py3-none-any.whl (98 kB)
Using legacy 'setup.py install' for cadquery, since package 'wheel' is not installed.
Installing collected packages: typish, casadi, typing-extensions, pyparsing, path, numpy, multimethod, nptyping, nlopt, ezdxf, cadquery
Running setup.py install for cadquery: started
Running setup.py install for cadquery: finished with status 'done'
Successfully installed cadquery-2.2.dev443+g162e3c5 casadi-3.5.6rc2 ezdxf-0.17.2 multimethod-1.8 nlopt-2.7.1 nptyping-2.0.1 numpy-1.22.4 path-16.4.0 pyparsing-3.0.9 typing-extensions-4.2.0 typish-1.9.3

$ python -c 'import cadquery; print(cadquery.version)'
2.1

@jmwright
Copy link
Member Author

@lorenzncode Thanks, I fixed the hard-coded version number and now the two following forms work.

$ python -c 'import cadquery; print(cadquery.__version__)'
2.2.dev444+g7cff170
$ python -c 'import cadquery; print(cadquery.version("cadquery"))'
2.2.dev444+g7cff170

Once we confirm that this works in the conda packages as well I'll probably add it to the GitHub issue templates so that we know what commit users are running when they file a bug.

@jmwright jmwright merged commit 893c799 into master May 23, 2022
@jmwright jmwright deleted the setup branch May 23, 2022 15:28
@jmwright
Copy link
Member Author

@adam-urbanczyk The SCM version number code gives an SCM version when installed via conda, but it's not the correct one. I actually can't track the conda version number to any recent commit.

conda version:

$ python -c 'import cadquery; print(cadquery.version("cadquery"))'
2.2.dev435+gddceab1.d20220520

pip version (installed via git repo):

$ python -c 'import cadquery; print(cadquery.version("cadquery"))'
2.2.dev445+g893c799

I'm not sure what is happening with the conda version number.

@fpq473
Copy link
Contributor

fpq473 commented May 23, 2022

The conda version is is referring to commit ddceab1 (the hash follows the g). The d20220520 says that the working directory was dirty, and that the build happened on 20220520. There have also been 435 commits since the tag preceding 2.2, i.e. 2.1.

More details: https://github.com/pypa/setuptools_scm#default-versioning-scheme

@jmwright
Copy link
Member Author

@fpq473 That's odd, I installed cadquery into a fresh conda environment and used "cadquery=master". Maybe the conda package indexes weren't updated yet? I'll retry the install tomorrow.

@jmwright
Copy link
Member Author

I tried a reinstall via conda this morning and it's still showing the same older commit. The files on anaconda.org have been updated according to their timestamps, and I don't see anything in the build template that's changing/setting the commit. Maybe setuptools_scm is getting confused because of the way conda is packaging the builds?

@fpq473
Copy link
Contributor

fpq473 commented May 24, 2022

Is the build log available? I can stare at it, but this does seem mystifying...

@adam-urbanczyk
Copy link
Member

Maybe, if it is not working correctly, it should be removed / implemented in a different way?

@jmwright
Copy link
Member Author

@fpq473 I figured out that the reason I was getting the strange version number for the conda install was because I was running the version code while I was cd'd into the cadquery directory with the conda environment active. That was causing issues because it was reading the SCM version of whatever branch I was on locally at the time rather than from conda's copy of CadQuery. When I'm in another directory I get version number 0.0.0.

@adam-urbanczyk I guess what I would propose would be to detect when the version number returned is 0.0.0, meaning an SCM version could not be determined, and then fall back to a hard-coded version number. That at least puts us back to where we were with the conda version before this PR, and still allows us to keep the SCM version functionality for situations where it's supported. Although, if there's a way to somehow tie the conda packages back to a commit as well, I would prefer that approach. I'm just not sure how to make that work.

@fpq473
Copy link
Contributor

fpq473 commented May 25, 2022

Not sure this helps, because I'm not following the conda building, but have you seen what @ovidner did in #924? That PR uses

export PACKAGE_VERSION=$(python setup.py --version)

to set the conda package version (I guess conda reads $PACKAGE_VERSION?).

% python setup.py --version
WARNING: The wheel package is not available.
WARNING: The wheel package is not available.
2.2.dev449+g8d10631
%

@jmwright
Copy link
Member Author

@fpq473 We could try adding that environment variable export to the conda templates. I'll look into it when I get a chance.

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.

None yet

4 participants