openscad icon indicating copy to clipboard operation
openscad copied to clipboard

Track all the issues/bugs of python-pr3-squashed PR

Open gsohler opened this issue 2 years ago • 37 comments

Let us here collect all the bugs found of python-pr3-squashed and address them so it becomes good for merge

  • [x] Python backup files shall have py suffix
  • [x] Address memory leaks in pyopenscad.cc and pyfunctions.cc
  • [ ] make all code more compact by using nanobind or pybind11
  • [x] imports are broken issue found by lkarthee
  • [x] funny bug with the increasing counter on each run
  • [x] missing str
  • [x] python function to execute an scad string (call scad from python)
  • [x] debug features like %. #. ! , possible using unary operators
  • [x] Explicit imports
  • [ ] explore if types can be introduced - 2d, 3d, etc
  • [x] Sync SCAD kwarg parameters for all functions example cube(dim=[]) to cube(size=[])
  • [ ] Add new positional parameters for function definitions like cube(x, y, z, centre)
  • [ ] how to handle deprecations, should they be left out.
  • [ ] how to handle feature flags. ???
  • [x] customizer story ??
  • [ ] autocomplete in editor
  • [x] Print nice warning if python interpreter is not installed on target system (instead of console crash)
  • [x] Using Embeddable python , so pythonscad works not standalone in windows without python installation

feel free to add more bugs (or rewrite the wording if its not clear enough)

gsohler avatar Dec 05 '23 20:12 gsohler

Took the liberty of fixing up the markup.

jordanbrown0 avatar Dec 05 '23 23:12 jordanbrown0

Relevant comments for context:

  • https://github.com/openscad/openscad/pull/4841#issuecomment-1839116916
  • https://github.com/openscad/openscad/pull/4841#issuecomment-1841130587
  • https://github.com/openscad/openscad/pull/4841#issuecomment-1841222912

lkarthee avatar Dec 06 '23 06:12 lkarthee

@gsohler can you add below things for tracking

  • explicit imports
  • explore if types can be introduced - 2d, 3d, etc
  • Sync SCAD kwarg parameters for all functions example cube(dim=[]) to cube(size=[])
  • Add new positional parameters for function definitions like cube(x, y, z, centre)
  • how to handle deprecations, should they be left out.
  • how to handle feature flags.
  • customizer story
  • autocomplete / hint in editor (editor shows hints for scad function definitions)

I have added last two to the list (i missed them yesterday).

lkarthee avatar Dec 06 '23 06:12 lkarthee

The counting variable bug is caused by interactions with the customizer. the customizer inserts the template variable value after python file is parsed, but its result is injected in the code in the very beginning. successive runs will increase the value. https://imgur.com/a/dLUk7Cp Maybe its better to parse the python code with regex instead of with python ?

gsohler avatar Dec 06 '23 10:12 gsohler

Multiple ways we can solve it :

  • write a python script using ast and tokenise modules to parse and generate customizer json. Then feed json to the existing customizer module. This is just me thinking out loud. I am sure we have to figure out few things along the way.
  • use an annotation PEP (i can't recollect) gets information about customizer variables.
  • add new code for parsing python in C++ customiser (last option)

My suggestion is to disable customizer for now.

lkarthee avatar Dec 06 '23 11:12 lkarthee

Most easy way would be just to accept variables for customizer which are declared as const , but there is no such keyword in python. Next solution would be to just accept variables which contain CONST in their name. then it should be obvious to the programmer that he should not change it

a=5 b=a b += 10

would also work

gsohler avatar Dec 06 '23 11:12 gsohler

Currently we are generating bindings and intializing a python interpreter inside OpenSCAD runtime and creating a tree. A non scad script just generates a CSG Tree if the program is valid and sends to OpenSCAD using DBus or writing into a temp file, etc. OpenSCAD will read CSG Tree and preview it or render it. This is a two step process but it decouples or turns OpenSCAD into plug and play infrastructure. Like workflow will be native - user will install python and install openscad package from pip. OpenSCAD side it will only have infrastructure to edit a python file, run the file and preview or render a CSG tree which is output.

@kintel @t-paul @pca006132 @jordanbrown0 @gsohler Is this technically possible or I have gaps in the story ?

lkarthee avatar Dec 06 '23 12:12 lkarthee

this is yet already possible. an external program/editor can save the scad file which is in use and openscad will automatically adapt to the new file content and display. having python inside openscad allows for even more interaction, allowing dataflow in both directions, not only TO openscad

gsohler avatar Dec 06 '23 12:12 gsohler

Maybe its better to parse the python code with regex instead of with python ?

No, absolutely not. This will cause you tons of trouble later...

Next solution would be to just accept variables which contain CONST in their name. then it should be obvious to the programmer that he should not change it

I think using a function is simpler.

import openscad.Customizer.Variable as Var
a = Var("a", 1) # name: a, default value: 1

This way you can even use spaces in the name.

A non scad script just generates a CSG Tree if the program is valid and sends to OpenSCAD using DBus or writing into a temp file, etc. OpenSCAD will read CSG Tree and preview it or render it.

If you need results from openscad, e.g. query object data, you need a full RPC for this. A C binding will be more flexible, much simpler to implement, and users can implement whatever RPC mechanism with that C binding if they want.

pca006132 avatar Dec 06 '23 12:12 pca006132

Customizer.Variable is a defined on CC side, right ?

gsohler avatar Dec 06 '23 12:12 gsohler

Yes, you need the C++ code to interact with the C++ API :)

pca006132 avatar Dec 06 '23 12:12 pca006132

Most easy way would be just to accept variables for customizer which are declared as const , but there is no such keyword in python. Next solution would be to just accept variables which contain CONST in their name. then it should be obvious to the programmer that he should not change it

We should not put restrictions on python programming. We should not introduce non native things which are not present in python programming. Immutability is not a python feature. If we need to change customizer we have to do it.

lkarthee avatar Dec 06 '23 12:12 lkarthee

I started trying with nanobind and came up with this CMakeLists.txt

nanobind_add_module( openscad NB_STATIC STABLE_ABI pymodule.cc)

it creates these modules libs: libnanobind-static.a openscad.cpython-311-x86_64-linux-gnu.so

how can i tell it to create an openscad static lib for linking to openscad ?

gsohler avatar Dec 06 '23 15:12 gsohler

I am not sure if linking statically is even possible, at least I have never tried it. I think the intended use case of the generated lib is to be imported into python.

pca006132 avatar Dec 06 '23 16:12 pca006132

OK, but then I am wondering what the STATIC means in NB_STATIC keyword. Right now nanobind does actually create a shared object file ready for use with python import. But then I am wondering how my "output" binding will work. This file defined in the python-openscad shared object module needs to set a variable which sits extern of the module. is this even possible ?

gsohler avatar Dec 06 '23 16:12 gsohler

OK I think I understand the problem now. See https://pybind11.readthedocs.io/en/stable/advanced/embedding.html

pca006132 avatar Dec 06 '23 17:12 pca006132

[ A single github issue may not be a good way to discuss design ideas for N different behaviors. ]

WRT having an external processor produce a CSG file that OpenSCAD displays, yes, that works, but is very limited. As @gsohler says, there would be no path for the external program to query the results of the CSG operations. (There is no way in OpenSCAD today, but this is a frequently-requested feature addressed in PR#4478.) Even the textmetrics feature available today would be awkward, because both sides would need to be doing exactly the same text layout. In addition, the existing "click on the model to jump to the source" feature would require significant work. (One could imagine annotating the CSG with source file and line numbers, much like #line in C.)

jordanbrown0 avatar Dec 06 '23 19:12 jordanbrown0

pca006132, apart that the document describes pybind11, wheras i started off with nanobind, the section describes, that its even possible to import embedded modules when i use their python interpreter functions instead of plain python code. Using that i will probably need even more specialized code to redirect python output to openscad console .. However i do not yet get which problem you have seen ?

gsohler avatar Dec 06 '23 19:12 gsohler

Problem: embedding the python interpreter.

I think although the documentation is about pybind11, it may work with nanobind. If it does not work, it may worth opening an issue there and see if they intended to support this. For more specialized code, I don't have much idea about this for now.

pca006132 avatar Dec 07 '23 02:12 pca006132

I started migration to nanobind in a new branch, so I am flexible so changing to nanobind is scheduled/planned. Lets see, when/if we will arrive. I'll raise a ticket @ Jakob

On Thu, Dec 7, 2023 at 3:29 AM pca006132 @.***> wrote:

Problem: embedding the python interpreter.

I think although the documentation is about pybind11, it may work with nanobind. If it does not work, it may worth opening an issue there and see if they intended to support this. For more specialized code, I don't have much idea about this for now.

— Reply to this email directly, view it on GitHub https://github.com/openscad/openscad/issues/4880#issuecomment-1844111385, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MQ3IKI3WBDLH3VLYHLYIESZXAVCNFSM6AAAAABAIJ3YGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBUGEYTCMZYGU . You are receiving this because you were mentioned.Message ID: @.***>

gsohler avatar Dec 07 '23 07:12 gsohler

seems not planned

https://github.com/wjakob/nanobind/issues/169

gsohler avatar Dec 07 '23 07:12 gsohler

They have it mentioned in their porting guide - https://nanobind.readthedocs.io/en/latest/porting.html#removed-features

There are two parts to python story - embedding an interpreter and creating bindings.

I have a suggestion say if we have a plain python interpreter embedded in app and it loads the bindings from the path will it work. Python loads modules from sys.path It should work technically right ?

Edit:

I have a suggestion say if we have a plain python interpreter embedded in app and it loads the bindings from the path will it work. Python loads modules from sys.path It should work technically right ?

I can confirm that this works. Can use plain python embedded interpreter and add library paths. It will load modules which are defined by nanobind, plain python modules from the path.

lkarthee avatar Dec 07 '23 07:12 lkarthee

@gsohler One another thing missing from this PR is packaging and shipping a python distribution so that OpenSCAD will be self contained. Can you add this to tracking list.

More about this here:

  • https://ubuverse.com/embedding-the-python-interpreter-in-a-qt-application/
  • https://discuss.python.org/t/embedding-python-in-a-c-self-contained-executable/25677

Currently python is dynamically linked to installed version from developer's environment. So that will not work for releases.

It will fail with below log:

Termination Reason:    Namespace DYLD, Code 1 Library missing
Library not loaded: /opt/homebrew/*/Python.framework/Versions/3.11/Python
Referenced from: <0EB8958E-EEE4-3592-9EA9-23A01C5810A4> /Users/USER/*/OpenSCAD.app/Contents/MacOS/OpenSCAD
Reason: tried: '/opt/homebrew/*/Python.framework/Versions/3.11/Python' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/*/Python.framework/Versions/3.11/Python' (no such file), '/opt/homebrew/*/Python.framework/Versions/3.11/Python' (no such file)
(terminated at launch; ignore backtrace)

At the minimum two things are needed Python libraries and std library. Without standard library, Python interpreter will fail to initialize as it does few basic things for importing few libraries like encodings and others.

Python path configuration:
  PYTHONHOME = (not set)
  PYTHONPATH = (not set)
  program name = '../OpenSCADEmbed'
  isolated = 0
  environment = 1
  user site = 1
  safe_path = 0
  import site = 1
  is in build tree = 0
  stdlib dir = '/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11'
  sys._base_executable = '/Users/USER/openscad/build/scripts/../OpenSCADEmbed'
  sys.base_prefix = '/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11'
  sys.base_exec_prefix = '/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11'
  sys.platlibdir = 'lib'
  sys.executable = '/Users/USER/openscad/build/scripts/../OpenSCADEmbed'
  sys.prefix = '/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11'
  sys.exec_prefix = '/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11'
  sys.path = [
    '/Users/USER/openscad/build/:/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python311.zip:/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11:/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/lib-dynload:/opt/homebrew/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages',
  ]
Fatal Python error: init_fs_encoding: failed to get the Python codec of the filesystem encoding
Python runtime state: core initialized
ModuleNotFoundError: No module named 'encodings'

Current thread 0x00000001e53adec0 (most recent call first):
  <no Python frame>

lkarthee avatar Dec 09 '23 04:12 lkarthee

Sorry i will Not duplicate all the required Files which come with the Python Installation. I can add nice Warning and Install Instruktion instead.

lkarthee @.***> schrieb am Sa., 9. Dez. 2023, 05:16:

@gsohler https://github.com/gsohler One another thing missing from this PR shipping a python distribution so that OpenSCAD will be self contained. Can you add this to tracking list.

More about this here:

https://ubuverse.com/embedding-the-python-interpreter-in-a-qt-application/

https://discuss.python.org/t/embedding-python-in-a-c-self-contained-executable/25677

Currently python is dynamically linked to installed version in developer's environment. So that will not work for releases.

It will fail with below log:

Termination Reason: Namespace DYLD, Code 1 Library missing Library not loaded: /opt/homebrew//Python.framework/Versions/3.11/Python Referenced from: <0EB8958E-EEE4-3592-9EA9-23A01C5810A4> /Users/USER//OpenSCAD.app/Contents/MacOS/OpenSCAD Reason: tried: '/opt/homebrew//Python.framework/Versions/3.11/Python' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew//Python.framework/Versions/3.11/Python' (no such file), '/opt/homebrew/*/Python.framework/Versions/3.11/Python' (no such file) (terminated at launch; ignore backtrace)

At the minimum two things are needed Python libraries and std library. Without standard library, Python interpreter will fail to initialize as it does few basic things for importing few libraries like encodings and others.

Python path configuration: PYTHONHOME = (not set) PYTHONPATH = (not set) program name = '../OpenSCADEmbed' isolated = 0 environment = 1 user site = 1 safe_path = 0 import site = 1 is in build tree = 0 stdlib dir = @./3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11' sys._base_executable = '/Users/USER/openscad/build/scripts/../OpenSCADEmbed' sys.base_prefix = @./3.11.5/Frameworks/Python.framework/Versions/3.11' sys.base_exec_prefix = @./3.11.5/Frameworks/Python.framework/Versions/3.11' sys.platlibdir = 'lib' sys.executable = '/Users/USER/openscad/build/scripts/../OpenSCADEmbed' sys.prefix = @./3.11.5/Frameworks/Python.framework/Versions/3.11' sys.exec_prefix = @./3.11.5/Frameworks/Python.framework/Versions/3.11' sys.path = [ @.@.@.@.***/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages', ] Fatal Python error: init_fs_encoding: failed to get the Python codec of the filesystem encoding Python runtime state: core initialized ModuleNotFoundError: No module named 'encodings'

Current thread 0x00000001e53adec0 (most recent call first): <no Python frame>

— Reply to this email directly, view it on GitHub https://github.com/openscad/openscad/issues/4880#issuecomment-1848220848, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MXWGJGFZSCXEJTEFJ3YIPQY5AVCNFSM6AAAAABAIJ3YGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBYGIZDAOBUHA . You are receiving this because you were mentioned.Message ID: @.***>

gsohler avatar Dec 09 '23 06:12 gsohler

Sorry i will Not duplicate all the required Files which come with the Python Installation. I can add nice Warning and Install Instruktion instead.

What would that warning be ?

lkarthee avatar Dec 09 '23 14:12 lkarthee

I think we should really think about what to ship with our release. The abi that pybind11 uses is not stable, that means it will not work when the python version is updated (or downgraded), so our binary will only work for a single python version that it is built together with. I don't think it is practical to ask users to install a specific version of python, considering this is usually something global and updating/downgrading their system-wide python may break something. I think we really need to ship the whole distribution. Not ideal but it can avoid a lot of problems.

Btw, @gsohler does pybind11 work for you?

pca006132 avatar Dec 10 '23 11:12 pca006132

pca006132 i am not happy with pybind11. e.g. i try to make a function to create a cube with 3 float parameters and it works when passing 3 floats. if i pass 3 int's instead, it does not convert them to float and fails, integer numbers really have to be written as 1.0 or 4.0 eg Also with pybind11, when working with own datatypes, you have to care about "Return value policies" which defined ownership an lifetime of the returned object whereas in bare embedded python you have to care about "New Reference" or "Borrowed Reference". If you get this wrong you get either memory leaks or segfaults in either case. I don't see the advantage of "getting it automatically correct with pybind11 and its harder for me to make a correct result with a generator instead of bare python. Also for bare python i can do much more customized error checking and error printing(errors in pybind11 always look very generic' ... personally I am not happy with pybind11 (and I don' t believe its my lack of knowledge because i had time to read all his docs in the homepage) Yes I can carefully go though all python API functions and get it correct, i just did not yet allocated the time for it.

gsohler avatar Dec 10 '23 20:12 gsohler

Blender includes an embedded Python. What do they do?

jordanbrown0 avatar Dec 10 '23 20:12 jordanbrown0

@gsohler Understood. For return value policies, it is not in the way when you use mostly std::shared_ptr, the default is usually fine. I think it is fine to use python APIs directly for now and revisit if we should change it to some binding generator/library later, it is mostly an implementation issue. The most important thing is that the current thing is comfortable to the one implementing it. @lkarthee what do you think?

@jordanbrown0 I just checked, blender on windows just bundles the whole python 3.10 distribution alongside with its libraries in their installation.

pca006132 avatar Dec 10 '23 23:12 pca006132

Everyone like Blender, Inkscape,etc who embeds a python interpreter, also bundle a python distribution, because :

  • applications need to be self contained, external dependency is a point of failure
  • its not uniform in windows, mac, linux - os bundled python version may be 2 or 3 or not ship any python.
  • there are multiple locations and multiple package managers (nuget, home-brew, mac python, asdf) through which python is installed, hence no uniform location where we can dynamically link to a python.
  • user might not be proficient in understanding all these dependency issues.
  • with dynamic linking, degrades quickly into only one way to work and multiple ways to fail.
  • have control over the version of python.

Blender and inkscape have python for extensions/addons. Blender has empowered users to create new menus, import export, automate user actions, etc

Even the author of pybind11 did not like complexity of it and he went on to create nanobind. With nanobind things are simplified and he dropped support for many things to achieve simplification and reduce complexity.

We don't need pybind11 as such, interpreter can be from plain Python, bindings can be from nanobind - https://github.com/openscad/openscad/issues/4880#issuecomment-1844821579.

@pca006132 I will leave it to Guenther and you.

lkarthee avatar Dec 11 '23 14:12 lkarthee