-
Notifications
You must be signed in to change notification settings - Fork 14
MVP for externalized Python #422
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?
Conversation
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.
Good job
ksml-runner/src/main/java/io/axual/ksml/runner/config/KSMLConfig.java
Outdated
Show resolved
Hide resolved
| - main | ||
| - external-python | ||
| workflow_dispatch: | ||
| permissions: |
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.
Please remove.
| } | ||
|
|
||
| private static String[] getFunctionCode(String[] code, String spaces, Path baseDirectory) { | ||
| if (code.length == 1 && code[0].startsWith("file:")) { |
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.
No validation for malformed file: references. Users might accidentally type file: (empty), file: foo.py (space after colon), or file:foo.py extra text.
Without validation, these produce confusing IOException messages instead of clear format errors.
Suggested fix:
var location = code[0].substring("file:".length()).trim();
if (location.isEmpty()) {
throw new TopologyException("Empty file path after 'file:' prefix");
}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.
Leaving for now until Monday's discussion.
| functions: | ||
| sensor_is_blue: | ||
| type: predicate | ||
| code: file:test-filter-external-python-function.py |
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'm not in favour of this language style. It requires special interpretation by KSML, where I would hope that we can do something similar through the following:
type: predicate
code: |
import test-filter-external-python-function
expression: values_is_blue(key, value)If we change anything in the language at all for this feature, then I suggest to add an import tag per function, which would look like this:
type: predicate
import: test-filter-external-python-function
expression: values_is_blue(key, value)We could even interpret the import tag as "string or list of strings" in KSML, possibly generating more import statements before the code block:
type: predicate
import:
- time
- test-filter-external-python-function
expression: values_is_blue(key, value)This way, we would let KSML generate the "import sourcefile" statement as part of the function's code itself, making it more Python driven again instead of KSML interpreter driven.
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.
IMHO this would make things more confusing for Python programmers, not clearer. They would be used to import module in the top of their Python source but now will have to remember that import: module in KSML generates the import for them.
Also, when editing the source in an external editor, the IDE does not have the import and won't be able to provide type hints (and likely will flag statements referring to the import as being in error)
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.
Yes I agree. Adding the import in KSML would be my second-best option, but still preferable to the file: tag in the code. My first preference would be to make it work using just import module in Python code.
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.
You have that option: configure modulesDirectory and import the module in your code and have that be the only way. Then drop file: support completely.
Maybe we should make clear what we want first, instead of trying to resolve it via PR discussions?
This PR adds support for externalized Python and syntax highlighting and code completion for KSML Python files:
code:snippets can reference externalfile:globalCode:snippets can reference externalfile: