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

Avoid creating a new internal interpreter if DLite is called from Python #907

Merged
merged 16 commits into from
Aug 23, 2024

Conversation

jesper-friis
Copy link
Collaborator

@jesper-friis jesper-friis commented Aug 10, 2024

Description

If DLite is called from Python, run the Python storage plugins from the calling interpreter.

Edit (new since first review): The suggestion in the comment below is now implemented. By default is the old behavior kept for now. The new behavior will be the default from v0.7.0.

When importing DLite from Python, the user will now get follow warning message printed to stderr:

Warning: Behavior singleInterpreter is not configured. It will be enabled by default from v0.7.0. The old behavior is
scheduled for removal in v0.9.0.

To configure the behavior, set environment variable DLITE_BEHAVIOR_singleInterpreter to a boolean value. True will
enable the new behavior and false will disable it.

You can also configure it from Python by setting dlite.Behavior.singleInterpreter or from C by calling dlite_behavior_set().

You can get rid of this warning by defining the environment variable DLITE_BEHAVIOR_singleInterpreter (or DLITE_BEHAVIOR).

  • Is the warning message understandable?
  • Should the description of the new behavior be included in the warning?
  • Is it too long (could e.g. be shortened by referring to the documentation)?
  • Is it OK to get this warning by default, or is it something the user explicitly should ask for to get (e.g. by defining an environment variable)?

Edit2: Shortened the warning message to:

Warning: Behavior <NAME> is not configured. It will be enabled by default from v0.7.0. See https://sintef.github.io/dlite/user_guide/configure_behavior_changes.html for more info.

Added new page about how to configure behavior changes to the user guide.


The motivation for this PR was to investigate the following benefits:

  • When DLite is called from Python, DLite is not explicitly shutting down any internal interpreter, which could solve the issues with segfault during shutdown.
  • This PR has the potential of improving error reporting from plugins.
    • Interactive testing indicates that the error reporting is not improved...

Both of these benefits should be investigated further. Possible drawbacks should also be checked.

Type of change

  • Bug fix & code cleanup
  • New feature
  • Documentation update
  • Test update

Checklist for the reviewer

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?

@jesper-friis jesper-friis mentioned this pull request Aug 10, 2024
9 tasks
@jesper-friis jesper-friis marked this pull request as draft August 10, 2024 15:33
@jesper-friis jesper-friis marked this pull request as ready for review August 12, 2024 09:08
@jesper-friis
Copy link
Collaborator Author

From discussion with @francescalb:

This is a fundamental change in DLite that we should be carefully thought and tested before merged in. At the same time, we believe that this PR provides a better solution for running plugins that will create less confusion in the long run.

To avoid having to maintain two development branches, we suggest the following:

  • By default, keep the old behaviour for now.
  • Add a new environment variable. When set, switch to the new behaviour.
  • Add tests for both the old and new behaviour.
  • Make a plan for when to switch to the new behaviour.

This way of handling possible breaking changes could be formalised and standardised. We are For example already using deprecation warnings, but not very formalised. We should add a plan for when removing deprecated features.

Can the new environment variable be use for other behaviour changes as well? Or is it simpler to make a series of related environment variables, one for each feature to change (and test for)? If we go for the latter, would DLITE_BEHAVIOUR_* be good prefix for such environment variables. There should be an index of such variables with of what behaviour changes they control. A list of these variables should also be grammatically accessible for matrix testing.

@@ -52,14 +52,18 @@ DLite-specific environment variables
- **DLITE_ATEXIT_FREE**: Free memory at exit. This might be useful to avoid
getting false positive when tracking down memory leaks with tools like valgrind.

- **DLITE_BEHAVIOR_<name>**: Enables/disables the behavior `<name>`.
- **DLITE_BEHAVIOR**: Enables/disables the all behaviors by default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, not al behaviours. Must be rewritten. This is only for behaviour changes, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This option actually turns on/off all behaviors by default. It used for testing (assuming that the behavior changes are independent).

@francescalb
Copy link
Collaborator

Where in the code is this warning included?

I think it is way to long. It should be one line and point to the documentation.

@jesper-friis
Copy link
Collaborator Author

Where in the code is this warning included?

I think it is way to long. It should be one line and point to the documentation.

It is defined here: https://github.com/SINTEF/dlite/pull/907/files#diff-dd7c9a4ea160143771ecce3a127eb8157401075c16cd8524b91d00caae1fbb36L125

@jesper-friis jesper-friis merged commit 51e8d6d into master Aug 23, 2024
14 checks passed
@jesper-friis jesper-friis deleted the single-interpreter branch August 23, 2024 12:48
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.

2 participants