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.

Sign up for GitHub

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

Jump to bottom

[DROOLS-5964] Implement PMML-Trusty/Scesim backend integration #3344

Closed
wants to merge 12 commits into from
Closed

[DROOLS-5964] Implement PMML-Trusty/Scesim backend integration #3344

wants to merge 12 commits into from

Conversation

gitgabrio
Copy link
Contributor

@gitgabrio gitgabrio commented Jan 26, 2021 •
edited
Loading

@danielezonca @yesamer @jiripetrlik

See https://issues.redhat.com/browse/DROOLS-5964

Please let me know if RuntNotifier RunListener usage inside PMMLRunnerTest could be improved.

Needed by kiegroup/drools-wb#1458

@yesamer
Copy link
Contributor

yesamer commented Jan 26, 2021

Hi @gitgabrio thank you for your PR.
Before review it, I think we need to discuss how to manage the impact of this PR on Kogito version of the editor.
Please consider that Kogito version of the editor currently supports version 1.8 only. This means that, after merging this PR, all scesim files managed in BC will be updated to 1.9 and therefore no more compatible with Kogito. This is a scenario we should avoid. My suggestion is to include a PR which enables the kogito scesim marshaller to manage at least version 1.8 and 1.9.

@gitgabrio
Copy link
Contributor Author

Hi @gitgabrio thank you for your PR.
Before review it, I think we need to discuss how to manage the impact of this PR on Kogito version of the editor.
Please consider that Kogito version of the editor currently supports version 1.8 only. This means that, after merging this PR, all scesim files managed in BC will be updated to 1.9 and therefore no more compatible with Kogito. This is a scenario we should avoid. My suggestion is to include a PR which enables the kogito scesim marshaller to manage at least version 1.8 and 1.9.

Thanks @yesamer !

@gitgabrio gitgabrio mentioned this pull request Jan 26, 2021
Draft
@gitgabrio gitgabrio marked this pull request as draft January 26, 2021 14:53
@gitgabrio
Copy link
Contributor Author

@danielezonca @yesamer
Switched to draft because it also need modifications on drools-wb.
Please let me know if you prefer to keep as draft until someone may complete the client-side part.

@sonarcloud
Copy link

sonarcloud bot commented Jan 26, 2021

Kudos, SonarCloud Quality Gate passed!

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

82.6% 82.6% Coverage
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2021

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 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

danielezonca
danielezonca reviewed Aug 26, 2021
View reviewed changes
...ation-api/src/main/java/org/drools/scenariosimulation/api/model/ScenarioSimulationModel.java
}

@XStreamAsAttribute()
private String version = "1.8";
private String version = "1.9";
Copy link
Contributor

Choose a reason for hiding this comment

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

@yesamer
Do you think this is a problem for client side marshaller or in general do you expect any impact on Kogito/BC side?

...rc/main/java/org/drools/scenariosimulation/backend/fluent/PMMLScenarioExecutableBuilder.java
public static final String PMML_RESULT = "pmmlResult";
public static final String PMML_MODEL = "pmmlModel";

private static final String SEPARATOR = FileSystems.getDefault().getSeparator();
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 it is better to normalise the path to use / instead of handling platform specific separator: with this code I expect that if the scesim file is created on Linux and executed then on a Windows machine it will fail

...o-simulation-backend/src/test/java/org/drools/scenariosimulation/backend/PMMLRunnerTest.java
KieContainer kieContainer = getKieContainer();
ScenarioRunnerDTO scenarioRunnerDTO = getScenarioRunnerDTO();
PMMLScenarioRunner runner = new PMMLScenarioRunner(kieContainer, scenarioRunnerDTO);
runner.run(getRunNotifier());
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding an assertion on the result also when it succeed?

@gitgabrio gitgabrio closed this Mar 17, 2022
@gitgabrio
Copy link
Contributor Author

Suspended - waiting for better indications

@gitgabrio gitgabrio deleted the DROOLS-5964 branch July 8, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielezonca danielezonca danielezonca left review comments

@yesamer yesamer Awaiting requested review from yesamer

@jiripetrlik jiripetrlik Awaiting requested review from jiripetrlik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gitgabrio @yesamer @danielezonca