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

Introduce a generic scripting profile #2872

Closed
wants to merge 11 commits into from

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Mar 26, 2022

Closes #2201
Depends on #2852

This introduces a profile that can be used on all channel types and all item types with all available script languages. Input/Output values can either be State, Command or String. In the latter case the profile tries to convert the String to an acceptable State/Command for channel/item.

@J-N-K J-N-K requested a review from a team as a code owner March 26, 2022 09:54
@wborn wborn added enhancement An enhancement or new feature of the Core awaiting other PR Depends on another PR labels Mar 26, 2022
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I only had a brief look and found a typo. 😉

Do you think these script profiles make script engine based transformations like transform.javascript obsolete? I'm asking this because of openhab/openhab-addons#12359.

@wborn
Copy link
Member

wborn commented Mar 26, 2022

Do you think these script profiles make script engine based transformations like transform.javascript obsolete? I'm asking this because of openhab/openhab-addons#12359.

I think a generic "SCRIPT" transformation provided by the core probably still makes sense so you can pick any scripting language of your chosing for labels and within rules of other scripting languages (i.e. bullets 1,2,3 in the Transformation Usage docs).

@J-N-K
Copy link
Member Author

J-N-K commented Mar 27, 2022

Good idea. I need to check how this can be implemented.

@J-N-K J-N-K changed the title Introduce a generic scripting profile [WIP] Introduce a generic scripting profile Apr 16, 2022
@J-N-K
Copy link
Member Author

J-N-K commented Apr 16, 2022

I re-added WIP because I would like to make use of the new transformation registry for providing the scripts.

@wborn wborn removed the awaiting other PR Depends on another PR label Apr 16, 2022
@wborn wborn added the work in progress A PR that is not yet ready to be merged label Apr 25, 2022
@J-N-K J-N-K force-pushed the feature-scriptingprofile branch from e21429a to 677ddc5 Compare May 1, 2022 09:37
@J-N-K J-N-K changed the title [WIP] Introduce a generic scripting profile Introduce a generic scripting profile May 1, 2022
@J-N-K J-N-K removed the work in progress A PR that is not yet ready to be merged label May 1, 2022
@J-N-K J-N-K force-pushed the feature-scriptingprofile branch from 59cc910 to 0927751 Compare May 1, 2022 12:26
@J-N-K
Copy link
Member Author

J-N-K commented May 26, 2022

@openhab/core-maintainers It would appreciate if this could make it in the next milestone so we have the chance to remove bugs before the release.

@kaikreuzer
Copy link
Member

I agree and planned to look at it the next days!

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Many thanks for this new profile!
I am not sure if I fully understood all constraints here, so please allow me some (maybe stupid) questions below.

@J-N-K
Copy link
Member Author

J-N-K commented May 29, 2022

In fact, it's not a script you put there but the reference to a script:

  • js:myscript.js for a JavaScript script in conf/transform (provided by the FileTransformationConfigurationProvider)
  • js:config:js:myscript for a managed JavaScript script (provided by the ManagedTransformationConfigurationProvider, which unfortunately has no UI support yet - this is where I would expect the script editor)

This makes it easy to re-use the same script at different places (e.g. as transformation in the sitemap or a rule and also as a profile). An example could be a mathematical calculation like a common logarithm or a script for complex formatting of a numeric value to a string.

The information of available scripts that can be used is already available in the transformation service (similar to what the MAP transformation/profile does), so the whole code in the transformation would need to be duplicated in the profile. IMO this doesn't make much sense, it's better to have it only at a single place with good test coverage.

@kaikreuzer
Copy link
Member

I am not convinced that we should further promote transformation services and their syntax here. Transformations are a construct from openHAB 1.x, mainly useful for textual configurations in item or sitemap files. They never made it to be supported in the UI. Instead, script support in the UI was done as shown above in the rule editor, which is imho pretty user friendly.
For those reasons, I'd want to avoid adding now a dependency from profile to transformation here. It also feels wrong to me to put the profile scripts in the "conf/transform" folder.

I'd be interested in the opinions from @rkoshak and @ghys (and of course the other @openhab/core-maintainers) from a user perspective here.

@J-N-K
Copy link
Member Author

J-N-K commented May 30, 2022

I don't think abandoning transformations is the way to go. Transformations are very common in a lot of places: some bindings (like MQTT, HTTP probably being the most popular), in sitemaps and in rules. They should have full UI support instead and with #2821 we added core-support to create transformation configurations in UI.

Also I don't think that adding the same script in a lot of places is user-friendly, as it requires manually changing the script at a lot of places. E.g. a transformation that formats a number of seconds to "HH:MM" format could be easily re-used and if you would like to change from "09:32" to "9:32", this should be super-easy to change it in one place.

Looking at the place where to add the files: I would prefer not to have to add a file at all (i.e. UI support as pointed out above), but having the profile "configuration" in the transform folder is an already known concept: org.openhab.transform.javascript, org.openhab.transform.map, org.openhab.transform.scale already use it. Removing that would not be easy to explain.

@rkoshak
Copy link

rkoshak commented May 30, 2022

Transformations are critical for certain low level bindings like MQTT, HTTP, Serial, etc. where the data comes in as a big XML, JSON, or some other structured blob of text that needs to be parsed to pull out the desired values.

In addition, one of the few things that profiles are good for (controversial opinion, I dislike profiles because there is too much overlap between them and rules which causes confusion on when it's appropriate to use one or the other) is taking an event from a Channel and transforming it to some other type of Item. Thus, one can, for example, change those read-only Switches from the Network Binding to Contacts (like they should have been in the first place) or add units or strip units from a numerical value and the like.

If I were to redesign the transformation service, I'd take what's done in the MQTT/HTTP binding with the filtering and chaining, pull that into the transform profile, and remove the binding/binding custom implementation of transformations so that all channels on all bindings can be transformed and it works the same everywhere. But that's a different discussion and I'm sure would cause other problems.

As for this PR, my biggest concern is it widens the overlap between rules and profiles. It will be used as a replacement for rules by some users, guaranteed. But I don't think that outweighs the benefits of having a scripting profile like this. There are many use cases where a profile really is the right way to go and being able to harness more than just JavaScript would be incredibly useful.

@kaikreuzer
Copy link
Member

@rkoshak I didn't want to trigger any discussion about dropping transformations here.
My point was rather that profiles are closer to rules than to transformations and they especially require decent UI support. Having to put js:config:js:true into a plain text input field as a configuration vs. putting true into a script editor in the UI imho makes a huge difference. I'd assume users to be confused, if ScriptActions in rules need a completely different configuration syntax then what the Script profile expects.
If we were to focus on textual configuration in DSLs, I'd be fine with transformation syntax, but I'd think in the UI we should do better.

@rkoshak
Copy link

rkoshak commented May 30, 2022

My point was rather that profiles are closer to rules than to transformations and they especially require decent UI support.

From the end user's perspective ignorant of any of the implementation details, I'm not sure that's the case. Except for the follow profile, all the rest are primarily focused on transforming a channel event in some way before it gets to the Item (e.g. taking any event and transforming it into a date time, performing hysteresis, out of range detection, etc.). It's a different kind of transformation than what we'd traditionally have in the transformation service since some really complicated behaviors are handled (e.g. converting a number value to an ON/OFF based on a hysteresis) but it's still a transformation when all is said and done (except for the pesky follow profile which isn't and I'm not sure what to make of that).

I agree that good UI support will be needed but I don't really see opening it up to where any of the installed scripting languages can be used instead of just ECMAScript 5.1 only (i.e. JavaScript Trnsformation) changes things fundamentally. In fact it helps since end users won't have to learn a new language from what they are using in rules.

As for the confusion, the confusion already exists because of the JS Transformation. I don't see this making that any worse. I also don't see it making that any better either. Of course, anything that can be done to make these script profiles look (from a UI perspective) like a rule Script Action the better from an end user's perspective.

@J-N-K
Copy link
Member Author

J-N-K commented May 30, 2022

Let me add some points regarding the configuration:

  1. I have removed the need for the first js:, this information is already present. I have creates Remove unnecessary script type configuration from script transformation #2987 for that.
  2. For a script in files, you only have to put the filename in the configuration, nothing else. This is exactly what we have for the other profiles.
  3. For managed configuration (which as I said above already has core support, but no UI support yet), the value is config:<scripttype>:<some_id>. My guess would be that someone who wants to configure a profile in an .items file also puts his scripts in files, so (2) is more likely what would be used.
  4. For UI configuration of the profile, a drop-down list is presented with all available configurations, that is what the ConfigOptionProvider is used for, so nothing has to be entered manually at all, just selected from the list.

@kaikreuzer
Copy link
Member

Thank you both for the insights, that's ok for me.

For UI configuration of the profile, a drop-down list is presented with all available configurations, that is what the ConfigOptionProvider is used for, so nothing has to be entered manually at all, just selected from the list.

This might actually be the reason for my confusion: I don't see any drop-downs, but as mentioned above just a plain text input field:

Screenshot 2022-05-30 at 22 55 55

Without any further documentation, I as a user am completely lost here of what I should enter and in which format.
What kind of values should I see here in the dropdown?

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
Copy link
Member Author

J-N-K commented Jun 4, 2022

I agree that profiles and transformations are two different things. In case of JavaScript Transformation/Profile and the (already merged) Script transformation and the profile proposed here, they are somewhat linked.

To be more precise:

  • The script transformation merged in Add a generic script transformation #2883 is intended to replace the JavaScript transformation and extend it to all other scripting languages.
  • The profile proposed here is intended to replace the JavaScript Profile (which is located in the same bundle as the transformation) and extend it to all other scripting languages.
  • Both (the JavaScript Profile and the Script Profile) are internally using the associated transformation service to reduce duplicated code.

So my point why it is like it is:

  • I don't see any good reason not to re-use existing, working, well-tested code instead of implementing exactly the same functionality a second time.
  • Re-using the transformation configurations as configurations in a profile is a concept that is already implemented for all other transformations and well accepted and understood by the user.

I also fail to see where the exiting JavaScript transformation has better UI support than what is proposed here:

Bildschirmfoto 2022-06-04 um 19 47 27

This is exactly the same, the only difference is that it ALSO allows in-line scripts when the configuration starts with '|'. That could be easily added.

I'm okay with moving it to 3.4 but I doubt that this will bring us any closer to a solution.

J-N-K added 2 commits June 4, 2022 21:22
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
Copy link
Member Author

J-N-K commented Jun 4, 2022

Latest code renders the UI like this:

Bildschirmfoto 2022-06-04 um 22 16 09

Two configurations (the ones with ".script") are added via files, the other one was added manually via the REST API. The red dots mark the place where I would like to see a "Add new script" button, to add new managed configurations. The editor should be the same you see when you add a script action to an UI based rule.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@kaikreuzer
Copy link
Member

The script transformation merged in #2883 is intended to replace the JavaScript transformation

At the moment it exists in co-existence, but in contrast to the JS transformation without any documentation (since it is not implemented as an add-on in the add-ons repo, although transformation services are an add-on type, so we have a pretty weird situation here, which we might want to clean up).

We should also agree on what "intended to replace" means: Do we want them to coexist or do we want it to replace it immediately to not have two competing implementations in place at the same time. In case of the latter, we need to communicate this to the user and should provide some migration path (either automatic or manual).

The profile proposed here is intended to replace the JavaScript Profile

As discussed above: I fully agree that it should replace the JS (transformation) profile, but with it not only this, but all other specific transformation profiles, so that the new one is a full replacement, i.e. a generic transformation profile.
Here again, we will need to find a way how to migrate existing users from the specific profiles over to the generic profile.

Besides this new transformation profile in this PR, I would imagine to have additionally a new Script profile as it had been suggested in #2201, i.e. using UI script editor, Blockly, etc. But this can clearly be implemented fully independently and won't raise any issues wrt migration as this does not yet exist.

It is mainly the question on how to properly migrate users to the new transformation profile and to make sure that the new format is stable and won't require another breaking change after releasing, which makes me think that it'll be safer to not rush anything right now, but do it cleanly and with a proper plan for 3.4.

@J-N-K
Copy link
Member Author

J-N-K commented Jun 5, 2022

"Replacement" here means: The JSTransformation/Profile will automatically be removed once we switch to Java 17 (because it depends on Nashorn). I would prefer to have this one in place BEFORE this happens, so users don't have to migrate immediately when upgrading, but can do that on their own schedule. But as I said before: I'm fine with delaying it to a time after the release. #2990 is independent from that and should be merged.

I still don't understand what the difference to your script profile is. It's just an UI issue to present the correct editor for adding new managed configurations in the right place. Then it's exactly the same like adding a script action to a rule in UI, it can even be blocky since blocky in the end creates a JS script which can be used here. The blockly editor just needs to be extended to allow using the input variable.

I did check if it is possible to make a generic transformation profile that can be used with all transformations (like MAP, SCALE, REGEX, XPATH, SCRIPT). It's not easy to do that (especially for UI), because depending on the selected transformation the UI would need to render different configuration parameters (e.g. MAP and SCALE provide a list of options, REGEX needs a regular expression, XPATH needs a path and a source format, SCRIPT needs the script type and provides a list of options).

@J-N-K
Copy link
Member Author

J-N-K commented Oct 27, 2022

@kaikreuzer Any chance for this to make it into 3.4?

@ccutrer
Copy link
Contributor

ccutrer commented Oct 27, 2022

Yesterday I wrote #3135 without realizing this PR existed (even though I've been pointed to it before! Whoops, I must have a bad short term memory.) My PR doesn't reuse transformations, but instead reuses the rule engine infrastructure. I've looked over this one a bit, and want to make some comparisons:

  • Because this one uses the transformation service, you can only pass in one variable (and not even a named one). Passing in the callback, as well as the ProfileContext (or at least the configuration from the ProfileContext) would allow the script to be far more useful and configurable. Also, in this PR's current iteration there's no way for the script to tell if it's being passed a command or a state update (though that might not be a problem - I'll admit I don't fully understand how/why a binding can send a command towards the framework; an item sending an update is more common, but most profiles don't do anything with that, though some do).
  • Because transformations can only return Strings, there's an extra step of converting back to a command (or even worse a state, with extra special cased code). This round-trip isn't necessarily lossless - especially for StringItems where you can't tell if NULL is supposed to be UnDefType or StringType.
  • I really like the script picker in the UI from your screenshot in Introduce a generic scripting profile #2872 (comment). I don't see how you enabled that though.
  • When using a script, MainUI already has full UI support for creating and editing them.
  • Why don't we just implement a TransformProfile, that allows using any transformation as a profile, allowing us to remove all of the *TransformationProfiles? That would also include the script transformation (which, agreed, needs better documented. Maybe I'll go make a commit to openhab-docs for that, since I explored using it already with ruby). I could see having both a ScriptProfile (my PR) that is more flexible for the reasons I listed above, and a TransformationProfile that accomplishes what this PR is looking to do (and more). The only downside is possibly a slightly more clunky UI. But if transformations are going to stay around, it'd be nice to have better UI support for them anyway. Especially when they're used on channels like in the MQTT binding as @rkoshak mentioned at Introduce a generic scripting profile #2872 (comment).

@J-N-K
Copy link
Member Author

J-N-K commented Nov 1, 2022

@ccutrer As I said above: It's very difficult to create a generic transformation profile, even if I would prefer to do that. The reason is that the transformations have different parameters and UI would need to be adapted in a way that depending on the selected transformation different configurations are shown.

@kaikreuzer
Copy link
Member

@kaikreuzer Any chance for this to make it into 3.4?

Sure, let's try that. But I first have to get my head around what exactly "this" is now supposed to be. 😄

I could see having both a ScriptProfile (my PR) that is more flexible for the reasons I listed above, and a TransformationProfile that accomplishes what this PR is looking to do (and more).

This is more or less what I argued above as well, although as a ScriptProfile, I would prefer the "inline" version over #3135, but this could possibly be best discussed in #3135 then.
In consequence, this PR here should not be about a "generic scripting profile", but about a "generic transformation profile".

I see your point @J-N-K that a full UI support of such a profile can be difficult to achieve. But a first step could be a simple string representation. After all, this is what all transformation add-ons describe in their documentation and how transformations are used within DSLs as well as in scripts. Having this string syntax being parsed and supported by the UI would for me be a (nice-to-have) second step that should not prevent the introduction of such a profile. Wdyt?

@J-N-K
Copy link
Member Author

J-N-K commented Nov 3, 2022

If this shall now be a generic transformation profile, then that is a completely different PR. And good UI support is why I created it the way it is now.

@ccutrer
Copy link
Contributor

ccutrer commented Nov 3, 2022

@ccutrer As I said above: It's very difficult to create a generic transformation profile, even if I would prefer to do that. The reason is that the transformations have different parameters and UI would need to be adapted in a way that depending on the selected transformation different configurations are shown.

Considering there is not currently deep UI support for transformations anywhere, it seems we could have two choices: just build a generic transformation profile with limited UI support. I.e. the same as the MQTT binding's transformation editor, where it just gives you a text box, and you have to know transformation syntax yourself. Or we build a deep UI experience for transformations concurrently with a generic transformation profile, and later we could have other parts of the UI use that (state patterns, MQTT binding configuration, etc.). It feels like you're pushing that we shouldn't release a transformation profile without a deep UI experience though?

@J-N-K
Copy link
Member Author

J-N-K commented Nov 4, 2022

This is clearly a step backward. Currently the file-based transformations are provided by a ConfigOptionProvider to the profile. That is not possible any longer if we merge all in one profile because the profile cannot decide which options to show as it has no knowledge which is the correct ConfigOptionProvider.

@kaikreuzer
Copy link
Member

This is clearly a step backward.

I would say, having transformations defined as a text string is the status quo. As mentioned above, this is also what is documented for all of our transformation add-ons.
I therefore agree with @ccutrer that a "generic transformation profile" can be decoupled from the question on how transformations can be supported in the UI in future. The profile is "just another profile" with a (or multiple) transformation as a parameter. Supporting transformations in the UI is imho an independent topic as we would want this also for item state descriptions, sitemap entries and other places.

the profile cannot decide which options to show as it has no knowledge which is the correct ConfigOptionProvider.

Hm, why not? Wasn't that the whole idea about introducing the TransformatioRegistry? For configuration parameters that refer to a transformation, we could allow (a) a custom transformation string and (b) one of the options provided by a TransformationConfigOptionProvider that simply lists the registered transformations from the registry.

@J-N-K
Copy link
Member Author

J-N-K commented Nov 5, 2022

The UI currently shows the available transformations in the profile. You can easily check that by adding the MAP profile to any link.

image

If you add a generic transformation profile that does not have an option for the transformation type, it get's even more confusing if we can't show what needs to be entered in the field (e.g. the regular expression for the REGEX transformation), especially if we show a long selection list with all available transformations.

Anyway, this is a completely different PR than what I presented here. Since nobody except me seems to see the benefit here, I'll close this PR.

@J-N-K J-N-K closed this Nov 5, 2022
@J-N-K J-N-K deleted the feature-scriptingprofile branch November 5, 2022 11:07
@kaikreuzer
Copy link
Member

If you add a generic transformation profile that does not have an option for the transformation type, it get's even more confusing if we can't show what needs to be entered in the field

This feature definitely makes sense and I think that should be a part of the UI implementation for configuration parameters with context "transformation". The UI should then clearly allow the selection of a transformation type and then depending on that selection show the specifics for that (again either by defining an "in-line" transformation directly or selecting one of the ones available in the TransformationRegistry). And the transformation type can always be deduced from the transformation string (if we use that for the start), so there should be no problem for adding UI support later on. The benefit of this over this PR is that the UI support is not specifically done for transformations in profiles, but available for transformations in general (in item state descriptions, sitemaps, etc.).

@wborn
Copy link
Member

wborn commented Nov 5, 2022

I think transformations are stateless and profiles can have state. So without having generic profiles the JS profile will break when we go to Java 17 (openhab/openhab-addons#12359).

@J-N-K
Copy link
Member Author

J-N-K commented Nov 5, 2022

As I already said: This is not possible. Depending on the transformation used there are different things that need to be put in the "function" field:

Jinja:

Template to be evaluated. For example: {{ value_json.device.status.temperature }}

Scale:

Filename containing the scale mappings.

Regex:

Regular expression to be applied on the state. Should contain a capture group whose outcome will be the result. For example: .*=(\\d*.\\d*).* extracts the 23.5 from temp=23.5°C

The UI uses the config description of the profile to render the inputs, using the description as hint. There is no way of changing the description of a parameter (the function) based on the setting of another parameter (transformation type).

It may be possible to use some JS in UI to filter the suggestions based on the selected transformation type, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic Profiles
5 participants