[SofaPython] Small fix & new features. #656

damienmarchal · 2018-05-16T06:17:48Z

This PR contains a set of small changes on SofaPython that we are using in our projects.

CHANGELOG:

[SofaHelper] add StringUtils::getStringCopy (because it was duplicated in several place of the plugin)
[SofaPython] add systematically sys.argv in PythonEnvironment
[SofaPython] add a very general getattr function in Binding_Node (to write node1.node2.node3.obj.data)
[SofaPython] add a getTarget in BaseObject (to return the plugins that contains a component)
[SofaPython] add a getComponentsFromTarget in the Sofa module
[SofaPython] add some minor function in Binding_Data
[SofaPython] add docstring support for the Sofa module
[SofaPython] change Binding_BaseContext.getObject to return None instead of a broken exception

There is tests for all features.

This PR:

builds with SUCCESS for all platforms on the CI.
does not generate new warnings.
does not generate new unit test failures.
does not generate new scene test failures.
does not break API compatibility.
is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

to avoid having this code duplicated all around.

The current behavior was to not set a sys.argv if there where not argument passed in the Sofa command line. The consequence was that in the python code it was needed to do if hasattr(sys, "argv"): ... Which is rather uncommon. This Commit change this behavior by setting systematically sys.argv and additionally fix typos (ending white spaces)

This code brings an unified syntax to query members of a Node. So you can write: root.child1.child2.child3.mechanicalobject.position instead of: root.getChild("child1").getChild("child2").getChild("child3").getObject("mechanicalobject").position In case of similar names the predecessance rule is: Data, Link, Child then Object.

This returns the list of categories an object belong to (as they are defined in in the GUI & Modeler).

… no object is found. The existing code is rising an exception (without properly setting the exception cause) but other getSomething methods of the binding returns None when the path query does not match so it is better not to rise en exception and be consistant.

docstring is the proper way to document python code, so let's use it.

damienmarchal · 2018-05-16T06:19:27Z

[ci-build][with-scene-tests]

jnbrunet

Everything seems good to me!

hugtalbot · 2018-05-17T08:05:30Z

2 tests are failing is this normal ?

damienmarchal · 2018-05-17T12:25:09Z

Cleary not. I'm investigating because I have no idea where the problem could come from.

OK. I Got it...it is an interesting one so I report the details. In the code comments.

damienmarchal · 2018-05-17T12:35:00Z

applications/plugins/SofaPython/PythonEnvironment.cpp

        for(const std::string& arg : arguments) {
            argv.push_back(arg.c_str());
        }
-
-        Py_SetProgramName((char*) argv[0]); // TODO check what it is doing exactly


This line is not correct... according to python documentation Py_SetProgramName want a char* to a persistant char* (it does not copy the data)).
This code is reading everywhere in memory but the memory access is not detected in our CI because the line is only exectued when runSofa is started with specific command line option (--argv). Which our CI does not.

In this PR we set-up the argument whatever the --argv option so the code is executed in the CI and (we are lucky) this make some tests to fails.

The fix is easy...we simply needs to have the program name in the PythonEnvironment and not a stack allocated variable.

I'm not sure exactly why we are setting the program name here. As per the python doc, it should be set one time before the first call to Py_Initialize and, as you said, should be persistent throughout the execution. Running a python file should not change this value as the interpreter stays the same (runSofa's internal interpreter or eventually an external one when we will be able to include the sofa lib inside an external python script).

damienmarchal · 2018-05-17T12:40:48Z

applications/plugins/SofaPython/PythonEnvironment.cpp

        for(const std::string& arg : arguments) {
            argv.push_back(arg.c_str());
        }
-
-        Py_SetProgramName((char*) argv[0]); // TODO check what it is doing exactly
-        PySys_SetArgv(argv.size(), (char**)argv.data());


I fear this line is also incorrect.

See PySys_SetArgvEx in python documentation: https://docs.python.org/2/c-api/init.html#c.PySys_SetArgvEx

…nctions

damienmarchal · 2018-05-17T15:16:36Z

@hugtalbot & @jnbrunet
I fixed the problematic code and added new tests for each of the added features.

…t) which containes the component. This is useful to build introspection tools using python. The corresponding test is also added.

So that we can get a target without having actually a instance of an object.

jnbrunet · 2018-05-18T08:20:35Z

applications/plugins/SofaPython/SofaPython_test/python/test_BindingNode.py

+    ASSERT_EQ( rootNode.child1.child2.dofs.position, [[1,2,3], [4,5,6]])
+
+    ## Check the 'generalized' setattr
+    rootNode.child1.child2.dofs.position = [[2,2,2], [4,4,4]]


I'm not sure how this line tests the setattr of a node? It seems to test the setattr of a MechanicalObject (and the getattr of the parent nodes).

Correct, fixed, thanks.

…plicated elements.

…e behavior.

damienmarchal · 2018-05-18T12:47:27Z

[ci-build][with-scene-tests]

jnbrunet · 2018-05-18T15:45:27Z

✔️ to merge with these changes

guparan · 2018-05-23T08:26:58Z

Output of the failing test on MacOS:

[----------] 1 test from Batch/Python_scene_test
[ RUN      ] Batch/Python_scene_test.sofa_python_scene_tests/0
[INFO]    [SofaPython] Added '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/Compliant/python' to sys.path
[INFO]    [SofaPython] Added '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/Flexible/python' to sys.path
[INFO]    [SofaPython] Added '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/image/python' to sys.path
[INFO]    [SofaPython] Added '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/Registration/python' to sys.path
[INFO]    [SofaPython] Added '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/RigidScale/python' to sys.path
[INFO]    [SofaPython] Added '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/SofaPython/python' to sys.path
[INFO]    [SofaPython] Added '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/SofaTest/python' to sys.path
[INFO]    [PluginManager] Loaded plugin: /Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/build/lib/libSofaPython.dylib
[INFO]    [Python_scene_test] running /Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/SofaTest/SofaTest_test/scenes/damping.py
ERROR: wrong number of arguments
[ERROR]   [PythonScript] IndexError: list index out of range
  File "/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/SofaTest/SofaTest_test/scenes/damping.py", line 15, in <module>
    DAMPING_COEF = float( sys.argv[1] )

[ERROR]   [SofaPython] Script (file:damping) import error
[ERROR]   [PythonScriptController(PythonScriptController)]  load error (file '/Users/sofa/jenkins_2/workspace/mac_clang-3.4_options_pr2/pr/all/applications/plugins/SofaTest/SofaTest_test/scenes/damping.py' not parsable)
../applications/plugins/SofaTest/Python_test.cpp:263: Failure
Failed
python error
[  FAILED  ] Batch/Python_scene_test.sofa_python_scene_tests/0, where GetParam() = 48-byte object <81-00 00-00 00-00 00-00 7E-00 00-00 00-00 00-00 00-4D 60-FA C4-7F 00-00 80-4D 60-FA C4-7F 00-00 10-4E 60-FA C4-7F 00-00 10-4E 60-FA C4-7F 00-00> (1051 ms)
[----------] 1 test from Batch/Python_scene_test (1051 ms total)

hugtalbot · 2018-05-23T08:27:08Z

One last test is failing (indice out of range) for Python_test

The problem was that runFile is used either in the SceneLoader and to load a controller but the behavior should be different regarding the way the sys.argv have been set. The solution was to revert (with small refactoring) to the initial runFile behavior but adding a call to setArgument in the python SceneLoader.

damienmarchal · 2018-05-23T20:12:38Z

I don't understand why the compilation is failing...
I wonder if this is not a CI problem related to bad interaction with this PR https://github.com/sofa-framework/sofa/pull/659/files

guparan · 2018-05-24T08:36:54Z

[ci-build][with-scene-tests]

damienmarchal · 2018-05-24T12:47:43Z

The failing tests are now fixed... ready when the compile is over ?

damienmarchal · 2018-05-25T12:06:16Z

@guparan & @hugtalbot ready to merge ?

damienmarchal · 2018-05-31T07:36:07Z

I restart [ci-build][with-scene-tests] to take into account the recent merge in master.
Then merge. if ok.

damienmarchal · 2018-06-01T07:57:31Z

So it is time to merge.

damienmarchal · 2018-06-01T07:58:40Z

[ci-build][with-scene-tests]

damienmarchal added 8 commits May 16, 2018 07:32

[SofaHelper] ADD StringUtils::getAStringCopy

d663f84

to avoid having this code duplicated all around.

[SofaPython] ADD isRequired/isReadOnly/getHelp in Binding_Data

3fa822a

[SofaPython] ADD a getCategories method to Binding_BaseObject

27bde04

This returns the list of categories an object belong to (as they are defined in in the GUI & Modeler).

[SofaPython] UPDATES the docstring for Binding_BaseContext.getObject()

a48a965

[SofaPython] ADD docstring support to the Sofa python module.

ec288cb

docstring is the proper way to document python code, so let's use it.

damienmarchal added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request labels May 16, 2018

[Compliant] UPDATE the registering of the binding to add a docstring.

14ceff6

guparan requested a review from jnbrunet May 16, 2018 08:49

jnbrunet approved these changes May 17, 2018

View reviewed changes

damienmarchal commented May 17, 2018

View reviewed changes

damienmarchal added 2 commits May 17, 2018 17:09

[SofaPython] FIX sys.argv bugs in PythonEnvironment.cpp

b2af4f9

See PySys_SetArgvEx in python documentation: https://docs.python.org/2/c-api/init.html#c.PySys_SetArgvEx

[SofaPython] ADD Tests to validate behavior of some python binding fu…

13489dc

…nctions

damienmarchal added 2 commits May 17, 2018 23:37

[SofaPython] ADD a getTarget function thats returns the plugins(targe…

73e495e

…t) which containes the component. This is useful to build introspection tools using python. The corresponding test is also added.

[SofaPython] ADD a getTargetsFor in SofaModule

df9094f

So that we can get a target without having actually a instance of an object.

jnbrunet reviewed May 18, 2018

View reviewed changes

damienmarchal added 2 commits May 18, 2018 12:33

[SofaPython] FIX Sofa.getComponentsFromTarget() that was returning du…

bcb5dae

…plicated elements.

[SofaPython] FIX test_BindingNode.py that was not testing the adequat…

a486ebd

…e behavior.

damienmarchal force-pushed the newFeatureSofaPython branch from 746f26e to cf4cc2a Compare May 23, 2018 19:40

hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels May 30, 2018

damienmarchal closed this Jun 1, 2018

damienmarchal reopened this Jun 1, 2018

damienmarchal merged commit 89071ab into sofa-framework:master Jun 4, 2018

guparan added this to the v18.06 milestone Jun 18, 2018

damienmarchal deleted the newFeatureSofaPython branch August 6, 2018 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SofaPython] Small fix & new features. #656

[SofaPython] Small fix & new features. #656

damienmarchal commented May 16, 2018 •

edited by guparan

Loading

damienmarchal commented May 16, 2018

jnbrunet left a comment

hugtalbot commented May 17, 2018

damienmarchal commented May 17, 2018 •

edited

Loading

damienmarchal May 17, 2018

jnbrunet May 17, 2018

damienmarchal May 17, 2018

damienmarchal commented May 17, 2018

jnbrunet May 18, 2018

damienmarchal May 18, 2018

damienmarchal commented May 18, 2018

jnbrunet commented May 18, 2018

guparan commented May 23, 2018

hugtalbot commented May 23, 2018

damienmarchal commented May 23, 2018

guparan commented May 24, 2018

damienmarchal commented May 24, 2018

damienmarchal commented May 25, 2018

damienmarchal commented May 31, 2018 •

edited

Loading

damienmarchal commented Jun 1, 2018

damienmarchal commented Jun 1, 2018

[SofaPython] Small fix & new features. #656

[SofaPython] Small fix & new features. #656

Conversation

damienmarchal commented May 16, 2018 • edited by guparan Loading

damienmarchal commented May 16, 2018

jnbrunet left a comment

Choose a reason for hiding this comment

hugtalbot commented May 17, 2018

damienmarchal commented May 17, 2018 • edited Loading

damienmarchal May 17, 2018

Choose a reason for hiding this comment

jnbrunet May 17, 2018

Choose a reason for hiding this comment

damienmarchal May 17, 2018

Choose a reason for hiding this comment

damienmarchal commented May 17, 2018

jnbrunet May 18, 2018

Choose a reason for hiding this comment

damienmarchal May 18, 2018

Choose a reason for hiding this comment

damienmarchal commented May 18, 2018

jnbrunet commented May 18, 2018

guparan commented May 23, 2018

hugtalbot commented May 23, 2018

damienmarchal commented May 23, 2018

guparan commented May 24, 2018

damienmarchal commented May 24, 2018

damienmarchal commented May 25, 2018

damienmarchal commented May 31, 2018 • edited Loading

damienmarchal commented Jun 1, 2018

damienmarchal commented Jun 1, 2018

damienmarchal commented May 16, 2018 •

edited by guparan

Loading

damienmarchal commented May 17, 2018 •

edited

Loading

damienmarchal commented May 31, 2018 •

edited

Loading