Skip to content

Conversation

@jbirddog
Copy link
Contributor

@jbirddog jbirddog commented Jan 27, 2023

Opening this as a draft while I test more and work on back end integration to start to gather feedback on the approach. Names and new public apis might be tweaked along the way. Edit: ready for review.

The current usage of task data to track the environment for the PythonScriptEngine is convenient but does lend itself to growing task data as the environment is copied from task to task. Larger workflows can run into issues with their task data size. This change provides a way for applications utilizing SpiffWorkflow to provide their own mechanism for tracking the state of the PythonScriptEngine between executions. If nothing is done the library works as it currently does.

Box was moved from the PythonScriptEngine proper to the PythonScriptEngineEnvrionment since custom environments may or may not want to Box.

If a custom environment is provided the application needs to properly restore it like it would for a PythonScriptEngine instance.

Copy link
Collaborator

@danfunk danfunk left a comment

Choose a reason for hiding this comment

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

I love this.

# 02110-1301 USA


class Box(dict):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like giving people the option of not using Box.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should deprecate box too. The ultimate goal here should be to provide a default that does nothing but execute and evaluate with the task data added to the context, with no additional changes to anything outside of execution/evaluation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a deprecation warning and a BoxedTaskDataEnvironment if consumers need/want it. Updated two tests that relied on Box to use this new environment.


self.globals = default_globals or {}
self.globals.update(scripting_additions or {})
def __init__(self, default_globals=None, scripting_additions=None, environment=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the environment is passed in, why would continue to accept default_globals and default_additions as separate arguments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

for backwards compatibility of course.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should log some sort of deprecation warning, so we can move people away from this in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the warning and updated tests that used default_globals/scripting_additions.

@@ -0,0 +1,112 @@
import copy

class BasePythonScriptEngineEnvironment:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the interface I always wanted for SpiffWorkflows Python execution. Really beautiful.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

89.3% 89.3% Coverage
0.0% 0.0% Duplication

@jbirddog jbirddog merged commit 6473749 into main Feb 2, 2023
@jbirddog jbirddog deleted the spiff_task_data branch February 2, 2023 14:08
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.

4 participants