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

Added notifyOnComplete parameter for KernelExecutor #291

Merged
merged 4 commits into from
Sep 6, 2024
Merged

Added notifyOnComplete parameter for KernelExecutor #291

merged 4 commits into from
Sep 6, 2024

Conversation

sok82
Copy link
Contributor

@sok82 sok82 commented Aug 16, 2024

Added notifyOnComplete parameter for code execution enabling to send model notifications only when all code execution completed and execution phase is defined

Possible solve for #290

…model notifications only when all code execution completed
@echarles
Copy link
Member

@sok82 Change look good overall.

Build on CI complains with src/components/output/OutputAdapter.ts(85,76): error TS2345: Argument of type 'boolean | undefined' is not assignable to parameter of type 'JSONObject | undefined'. Type 'boolean' is not assignable to type 'JSONObject'. Is this related to changes introduced in this PR?

@echarles echarles self-requested a review August 23, 2024 09:55
@echarles echarles added the feature New feature or request label Aug 23, 2024
@echarles
Copy link
Member

@sok82 another PR builds fine, so I think the failure is coming from these changes. I would prefer if possible to have it fixed before merging.

@sok82
Copy link
Contributor Author

sok82 commented Sep 6, 2024

Hey @echarles,
sorry for waiting - was on the mountain trip without network coverage for long time(

I committed a fix for the error in this commit - 5fb5027

Some new parameter metadata of type JSONObject was introduced in OutputExecutor since I made my changes.

I had to just pass an empty metadata object to make it compile cause I don't have anything similar in context, not sure if it's correct.

Pls check

@echarles
Copy link
Member

echarles commented Sep 6, 2024

  • was on the mountain trip without network coverage for long time(

Sounds super cool

Changes lgtm, still an build error on the CI

src/components/output/OutputAdapter.ts(86,41): error TS2693: 'JSONObject' only refers to a type, but is being used as a value here.

@sok82
Copy link
Contributor Author

sok82 commented Sep 6, 2024

Sorry, had to work in blind mode - committed fix for this - src/components/output/OutputAdapter.ts(86,41): error TS2693: 'JSONObject' only refers to a type, but is being used as a value here.

Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

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

LGTM Thx @sok82

@echarles echarles merged commit a7afb26 into datalayer:main Sep 6, 2024
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants