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

[Binding/Sofa.Config] Add Sofa.future & Sofa.Config #184

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

damienmarchal
Copy link
Contributor

@damienmarchal damienmarchal commented Sep 15, 2021

Implement a control mecanism to activate/de-activate feature of the SofaPython3 binding so we have more freedom
to experiment.

Example of use (taken from PR #173):

from Sofa.Lifecycle import __feature__
def createScene(root):
     with __new_feature__("object_auto_init",True):
           root.addObject("MechanicalObject", position=[12,3])  # init() is now called automatically

The mechanisme is composed of three parts:

  • SofaPython3::Lifecycle (the c++ part)
  • SofaPython3::Binding::Lifecycle (the python binding of the c++ Config)

To add a new feature:

In binding/Sofa/package/lifecycle.py, add you feature at the begnining as indicated
in the file.

Then in your c++ code you can then test if the feature is activated by doing

if( sofapython3::lifecycle::features::get("global_auto_init") )
	....

Or in your python code you can also test if the feature is activated by doing

import Sofa.Lifecycle
if Sofa.Lifecycle.has_feature("global_auto_init"):
	print("The feature is activated")

To control if users want to activate news features in the binding.
…tures.

The mechanisme is composed of three parts:
- SofaPython3::Config (the c++ part)
- SofaPython3::Binding::Config (the python binding)

Activating an existing futurefeature:
-------------------------------------
```python
import SofaPython3.__futurefeatures__
SofaPython3.__futurefeatures__.object_auto_init = True # To activate the auto init feature.
```

Adding a new futurefeature:
---------------------------
In binding/Sofa/package/__futurefeatures__.py, add you feature at the end of the
FeatureSet::__init__ constructor.

In your c++ code you can test if the feature is activated by doing
```c++
if( sofapython3::config::futurefeatures::get("myFeature") )
	....
```

In your python code you can test if the feature is activated by doing
```python
import Sofa.__futurefeatures__
if Sofa.__futurefeatures__.myFeature:
	print("The feature is activated")
```

Signed-off-by: Damien Marchal <[email protected]>
@damienmarchal
Copy link
Contributor Author

@hugtalbot here is the feature. I think it should be used at least in PR #173 and few other.

…ture

# Conflicts:
#	bindings/Sofa/tests/PythonModule_Sofa_test.cpp
…time.future

To make that consistant with the "lifetime" namespace we already have in
Sofa.
@hugtalbot
Copy link
Contributor

hugtalbot commented Sep 27, 2021

@damienmarchal you are changing the name in the latest commit right?
Why lifetime ? I just see a lifecycle in SOFA but related to the componentState, therefore unrelated to this Future feature.

Could you explain ?
I thought Future was a standard keyword in the python world

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Oct 6, 2021

Oops, I missread "lifecycle" in the sofa code base (which is not related to componentState).

Future is the common name in python...but as we already have a namespace for feature management and software lifecylce management in Sofa I think it is better to stick to the sofa namespace.

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Oct 14, 2021

@hugtalbot I now rename everything so it match the sofa naming convention: lifecycle.

I also renamed the contextmanager __feature__ into __new_feature__... in case one day we decide to have something like __old_feature__ if one day we decide to provide on demand backward compatibility.

EDIT: The CI is failing because we need to merge PR ##206

@guparan
Copy link
Contributor

guparan commented Oct 25, 2021

Hi @damienmarchal
I have a few questions regarding this PR.

  1. Adding features in lifecycle::features
    • Why? What is considered a "new feature" and thus should be registered in lifecycle::features? What are the criteria?
  2. Removing features from lifecycle::features
    • How? Are all lifecycle::features meant to be used as normal features at some point?
    • Why? What are the agreed reasons for removing features? The expiration date (if any)? Something else?
    • When? How long does a feature stay registered in lifecycle::features? Is there an expiration date?
    • Who? Who is supposed to do that? Who is in charge of maintaining the lifecycle::features list?
  3. Other questions
    • Could we have a message telling the user he is using a "new_feature"? He might not know if he runs the scene of someone else.

Thanks.

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Oct 25, 2021

Nice day, I Have questions about a pending PR
Thanks @guparan

Adding features in lifecycle::features

Q: Why? What is considered a "new feature" and thus should be registered in lifecycle::features? What are the criteria?
A: at first sight I would say that two good candidates for explicit activations of new feature using context manager are:

Apart from that I would not recommand to use this kind of mecanism for integration of new feature that are neither breaking nor fragile.

Removing features from lifecycle::features

Q: How? Are all lifecycle::features meant to be used as normal features at some point?
A: By adding a context manager working the reverse way as new_feature.
Example of evolution of the "init by default" behavior in Sofa.
As it is breaking we first deploy it through explicit activation by users.

## I really want the new behavior. 
with Sofa.lifecycle.__new_feature__("auto_init"):
       createPartOfSceneNewStyle(root)
createPartOfSceneWithOldStyle(root)

When the feature is integrated, there is no need for the with statement. If the with new_feature is used we can now emit a warning explainging this is not needed anymore as the feature is now a properly supported one.

createPartOfSceneNewStyle(root)

Finally, if the scene want to continue using some parts with the old behavior we can explicitly fall back to the old behavior.

createPartOfSceneNewStyle(root)

## I'm to lazy to update my scene but I need to make it work now !
with Sofa.lifecycle.__deprecated_feature__("no_auto_init"):   
      createPartOfSceneWithOldStyle(root)

Thanks to this kind of lifecyle mecanism we then have a the infrastructure to handle a smooth transition and api evolution (which compared to the no-effort we have done for python2 to pyton3 is a big improvement).

Q: Why? What are the agreed reasons for removing features? The expiration date (if any)? Something else?
A: The decision to remove a feature is strictly the same as for the other parts of Sofa. In general this is when the sofa-dev team agreed it is time for that and when there is alternatve and a good migration plan.

Q: When? How long does a feature stay registered in lifecycle::features? Is there an expiration date?
A: Same as for other sofa lifecyle/deprecation/removal dates, this is decided by the dev-team.

Q: Who? Who is supposed to do that? Who is in charge of maintaining the lifecycle::features list?
A: Same as for other sofa lifecycle management. In general this is the one that make a PR introducing a breaking feature, or removes a deprecated one that is in charge of adding the transitional strategy as part of the PR.

Q: Could we have a message telling the user he is using a "new_feature"? He might not know if he runs the scene of someone else.
A: Every message combination is possible. I will make a proposal in which we emit msg_info().

@hugtalbot
Copy link
Contributor

Hi @damienmarchal

features that breaks compatibility with existing code base

This sounds to me like the master branch isn't it ? I would not really agree on this criteria.

features that are not breaking but that exposes an API that we know in advance will change but don't know when and how

Agreed here, but it sounds rather like something experimental, since unsure to be finally merged in master. Don't you think?
I also do not think #148 falls into this case.

Regarding #173, the question is rather to me do we really want this feature in SP3?
I can not see why an automatic init should be hidden. Anyone else @sofa-framework/reviewers-sofapython3 has an opinion on this one?

@guparan guparan modified the milestones: v21.06, v21.12 Oct 26, 2021
@hugtalbot
Copy link
Contributor

hugtalbot commented Oct 27, 2021

(From SOFA dev) During the SOFA dev meeting, the team agreed on setting up:

  • a mechanism to smoothly integrate new breaking features in SofaPython3 by putting it behind a ON/OFF switch (ON by default). It should always be coupled with a deprecated mechanism if the new feature changes/breaks a previous API (“new_feature”). This mechanism should always come with a due date.
  • a mechanism to test experimental features. Similar to the above but OFF by default. Experimental features are new features but which implementation does not cover the whole code, or which are not yet validated to be integrated in the code base (“experimental”)

TODO: add the deprecated mechanism + make sure we must set up dates when adding a new/deprecated feature + add the experimental feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants