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

where are the compas.geometry.trimesh* dependencies defined? #1334

Open
jf--- opened this issue Apr 23, 2024 · 15 comments
Open

where are the compas.geometry.trimesh* dependencies defined? #1334

jf--- opened this issue Apr 23, 2024 · 15 comments

Comments

@jf---
Copy link
Contributor

jf--- commented Apr 23, 2024

while compas is in an overall terrific state and a mature library, compas.geomtry.trimesh* is a glaring exception.
can we consider whether to move it out of the core lib into a module of its own?

@tomvanmele
Copy link
Member

(almost) all of these functions receive their implementation from plugins.

@tomvanmele
Copy link
Member

that said, the pluggable/plugin mechanism is in this case certainly not as useful as for example in the case of Brep and Nurbs...

@jf--- jf--- changed the title compas.geomtry.trimesh* is a mess(h)y at best where are the compas.geomtry.trimesh* dependencies defined? Apr 24, 2024
@jf--- jf--- changed the title where are the compas.geomtry.trimesh* dependencies defined? where are the compas.geometry.trimesh* dependencies defined? Apr 24, 2024
@jf---
Copy link
Contributor Author

jf--- commented Apr 24, 2024

(almost) all of these functions receive their implementation from plugins

right, and there is a neat docstring with no body...

what I find hard to grasp where trimesh or the plugin is defined.
since trimesh isn't mentioned in requirements.txt
since trimesh is well maintained, it should be included IMO: now this code just looks too magical.

  • I realise the plugin mechanism is there to differentiate from CPython vs IronPython
    since requirements.txt can be ran conditionally I don't see why these shouldn't be listed?

  • why not make the dependency explicit in the docstring? linking to the documentation of the method invoked.

  • the trimesh_geodistance_numpy.py file seems missing?

    from .trimesh_geodistance_numpy import trimesh_geodesic_distances_numpy

    if method == "exact":
        raise NotImplementedError

    return trimesh_geodesic_distances_numpy(M, [source])
  • it seems that trimesh* aren't covered in the test suite?

  • same seems to apply to triangulation_delaunay.py, where constrained_delaunay_triangulation is core functionality for RhinoVault IIRC?

  • since the root of the compas.geometry directory contains the core primitives, it's less then optimal to have so many more (shallow) trimesh* files in the directory. let's move these to a trimesh folder and remove the trimesh prefix?

  • trimesh_parametrisation.py basically contains 2 methods that raise a NotImplementedError?

@tomvanmele
Copy link
Member

the plugins have nothing to do with trimesh. some of them are defined in compas_cgal, some in compas_libigl. the modules are prefixed with trimesh, because they deal with triangular meshes exclusively.

see here for an example: https://github.com/compas-dev/compas_cgal/blob/dd9f32aade7e81e8c9f941885545dc4042bbec45/src/compas_cgal/slicer.py#L11

i noticed a few broken code paths indeed. have already fixed them and will send a PR.

the explicit registration of plugins for pluggables has been discussed on several occasions in the past. we can start the discussion again based on this thread...

in addition to the explicit registration of the (known) plugins for each pluggable, the goal is to have a default implementation for all of them (ideally in pure Python) that might be slow or less robust but is always available.

@gonzalocasas
Copy link
Member

One of the main reasons why we don't register known plugins in core is licensing. Adding an explicit registration would put in us in a grey zone in terms of how the plugin's license affects core's license. This is particularly the case with stuff like cgal and igl that have strong copyleft licenses (sometimes paired with a more liberal one in a dual licensing model).

Besides that, I find that from a software architecture point of view, registering a dependency on the "parent's side" creates an ugly circular dependency situation that is not clean to manage.

@jf---
Copy link
Contributor Author

jf--- commented Apr 24, 2024

Thanks for the reference to the example @tomvanmele , that's insightful.
compas_cgal & compas_libigl are excellent and I'm very aware of them.
I raised perhaps too many points at once.

I'm curious to have your thoughts on conditional requirements.txt
This seems a biggy to me: it'll allow to state dependencies explicitly.
I think this is key, since it makes the need for the plugin mechanism explicit too.

@jf---
Copy link
Contributor Author

jf--- commented Apr 24, 2024

stuff like cgal and igl that have strong copyleft licenses

gotcha

registering a dependency on the "parent's side" creates an ugly circular dependency situation that is not clean to manage

I'm not contesting the usefulness of a plugin mechanism. compas.plugin seems to echo pytest's [1] plugin architecture to some extent?

[1] any reference to pytest is a celebration of course ;)

@gonzalocasas
Copy link
Member

While pip can define conditional requirements, conda cannot, so it still wouldn't provide a full solution.
Maybe the simple and good-enough alternative is more documentation

@gonzalocasas
Copy link
Member

compas.plugin seems to echo pytest's architecture?

indeed, it's partially inspired by it

@jf---
Copy link
Contributor Author

jf--- commented Apr 24, 2024

(pip)

While pip can define conditional requirements, conda cannot, so it still wouldn't provide a full solution.

environment.yaml allows for a pip install block: they're complementary, see here
You basically can defer to a requirements.txt.
Looking at the selectors that pip offers, it seems reasonable to assume IronPython can be detected.


(conda)

conda definitely supports conditional requirements, not the best reference but see here

See here for the selectors -- so many architectures, except for our "beloved" Iron(y)Python.


(conda+pip)

Still, deferring to requirements.txt in a environment.yml is the way fwd IMO, since otherwise dependencies will be listed twice, which introduces ambiguity. ( compas_cgal seems to adhere to this mechanism

@gonzalocasas
Copy link
Member

Ah sorry, I used the wrong term. I didn't mean conditional but optional requirements.

So, I thought we were referring to the possibility of doing something like conda install compas[full_including_cgal_and_stuff]. That optional requirement doesn't work for conda as far as I know.

Creating an environment file definitely allows to fallback to pip, but it doesn't solve the problem because 1) it's not the same as a conda recipe, e.g. we lose the conda install compas one-liner, 2) it will actually use pip to install stuff, and we use conda exactly to avoid using pip because some stuff is difficult to install (on Windows) using purely pip.

It's technically possible to provide an env file and instruct people to do conda env create -f https://compas.dev/envs/compas_full/environment.yml but it's not very composable because one cannot have an existing environment and install compas on it that way.

One alternatively would be to create bundle packages, much like ROS does, we could have conda install compas.full.form_finding or conda install compas.full.robotics, etc. Those could be simpler meta-packages that just install many other compas package in one go.

@jf---
Copy link
Contributor Author

jf--- commented Apr 24, 2024

That optional requirement doesn't work for conda as far as I know

aha, yes I think so too...

we lose the conda install compas one-liner

by no means I'm challenging conda: I introduced it to the pythonocc project many moons ago -- imagine the horrors of building a CAD kernel via pip.

I don't follow your comment: for compas dependencies are stated only in requirements.txt
I think the current practice is that conda is using pip under the hood as is 💣😉

please see the earlier comment wrt compas_cgal.
I nice way to work with compas_cgal is to run conda env update -f=env_mac.yml followed by pip install -e .
The point in case: there's a soft, malleable boundary between the two tools....

@tomvanmele
Copy link
Member

i don't think the conditional requirements help in this case. compas_occ, compas_cgal and compas_libigl cannot be installed with pip (although this might change). therefore, we can't specify them as optional for a pip installation.

in the case of conda we can indeed use an environment file with all sorts of conditionals and additional pip instructions. this is already the case for packages like compas_cgal and compas_libigl. however this doesn't change anything about how compas or any other package is built on conda-forge.

if you want compas in combination with other packages to get some of the plugins in the same environment, you can simply do conda create -n combo compas compas_cgal compas_occ compas_libigl.

perhaps the solution is to simply reference the plugins from the docs, clearly stating that their installation and related licensing is the responsibility of the user, and add an environment file to the compas repo that allows users to install compas with all available plugins if they want, for convenience.

@gonzalocasas
Copy link
Member

perhaps the solution is to simply reference the plugins from the docs, clearly stating that their installation and related licensing is the responsibility of the user, and add an environment file to the compas repo that allows users to install compas with all available plugins if they want, for convenience.

+1

@jf---
Copy link
Contributor Author

jf--- commented Apr 24, 2024

perhaps the solution is to simply reference the plugins from the docs, clearly stating that their installation and related licensing is the responsibility of the user, and add an environment file to the compas repo that allows users to install compas with all available plugins if they want, for convenience

sorry for all the technicalities, yes that is the point, to have a path that in CPython its easy to get a complete installation.
apologies, it took a bit to grasp the circularity of the interdependencies

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

No branches or pull requests

3 participants