-
Notifications
You must be signed in to change notification settings - Fork 50
Patching #524
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
base: main
Are you sure you want to change the base?
Patching #524
Changes from all commits
a837a2e
8464b2a
2168762
e98f607
15d3b71
73e671a
bec1f82
9ad15e8
cae0669
f815478
0c7c492
276416c
e7f85a5
b8371ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -341,6 +341,7 @@ def __init__( | |
| self.conductor_key: Optional[str] = conductor_key | ||
| if config.get("conductor_key"): | ||
| self.conductor_key = config.get("conductor_key") | ||
| self.enable_patching = config.get("enable_patching") == True | ||
| self.conductor_websocket: Optional[ConductorWebsocket] = None | ||
| self._background_event_loop: BackgroundEventLoop = BackgroundEventLoop() | ||
| self._active_workflows_set: set[str] = set() | ||
|
|
@@ -350,6 +351,8 @@ def __init__( | |
| # Globally set the application version and executor ID. | ||
| # In DBOS Cloud, instead use the values supplied through environment variables. | ||
| if not os.environ.get("DBOS__CLOUD") == "true": | ||
| if self.enable_patching: | ||
| GlobalParams.app_version = "PATCHING_ENABLED" | ||
| if ( | ||
| "application_version" in config | ||
| and config["application_version"] is not None | ||
|
|
@@ -1525,6 +1528,50 @@ async def read_stream_async( | |
| await asyncio.sleep(1.0) | ||
| continue | ||
|
|
||
| @classmethod | ||
| def patch(cls, patch_name: str) -> bool: | ||
| if not _get_dbos_instance().enable_patching: | ||
| raise DBOSException("enable_patching must be True in DBOS configuration") | ||
| ctx = get_local_dbos_context() | ||
| if ctx is None or not ctx.is_workflow(): | ||
| raise DBOSException("DBOS.patch must be called from a workflow") | ||
| workflow_id = ctx.workflow_id | ||
| function_id = ctx.function_id | ||
| patch_name = f"DBOS.patch-{patch_name}" | ||
| patched = _get_dbos_instance()._sys_db.patch( | ||
| workflow_id=workflow_id, function_id=function_id + 1, patch_name=patch_name | ||
| ) | ||
| # If the patch was applied, increment function ID | ||
| if patched: | ||
| ctx.function_id += 1 | ||
| return patched | ||
|
|
||
| @classmethod | ||
| def patch_async(cls, patch_name: str) -> Coroutine[Any, Any, bool]: | ||
| return asyncio.to_thread(cls.patch, patch_name) | ||
|
|
||
| @classmethod | ||
| def deprecate_patch(cls, patch_name: str) -> bool: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with this approach is it's confusing where to run
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a better design is to explicitly register patches and explicitly enable/disable them. It's easier for testing too -- you can test the patched code controlling which patch is effective. |
||
| if not _get_dbos_instance().enable_patching: | ||
| raise DBOSException("enable_patching must be True in DBOS configuration") | ||
| ctx = get_local_dbos_context() | ||
| if ctx is None or not ctx.is_workflow(): | ||
| raise DBOSException("DBOS.deprecate_patch must be called from a workflow") | ||
| workflow_id = ctx.workflow_id | ||
| function_id = ctx.function_id | ||
| patch_name = f"DBOS.patch-{patch_name}" | ||
| patch_exists = _get_dbos_instance()._sys_db.deprecate_patch( | ||
| workflow_id=workflow_id, function_id=function_id + 1, patch_name=patch_name | ||
| ) | ||
| # If the patch is already in history, increment function ID | ||
| if patch_exists: | ||
| ctx.function_id += 1 | ||
| return True | ||
|
|
||
| @classmethod | ||
| def deprecate_patch_async(cls, patch_name: str) -> Coroutine[Any, Any, bool]: | ||
| return asyncio.to_thread(cls.deprecate_patch, patch_name) | ||
|
|
||
| @classproperty | ||
| def tracer(self) -> DBOSTracer: | ||
| """Return the DBOS OpenTelemetry tracer.""" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this config. Because users should be able to use patching + versioning together. This code forces the version to be a weird
PATCHING_ENABLEDstring which is not intuitive.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes behavior much more intuitive. Patching requires a static version string. This check provides clean errors if you don't have one. You can still override it if you really want to. Otherwise you'll be thinking you're using patching, but your workflows don't recover and you don't know why.