-
Notifications
You must be signed in to change notification settings - Fork 285
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
feat(ccmodel-hepheastus): add Hepheastus plugin #3641
feat(ccmodel-hepheastus): add Hepheastus plugin #3641
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.
@RafaelAPB I left some suggestions in the code, but it's fine by me if you'd prefer to address those later or never. I want to make sure I'm not slowing down the development.
...ages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/pm4py-adapter/check_conformance.py
Outdated
Show resolved
Hide resolved
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
@petermetz @RafaelAPB I also added a quick message about the python dependency in the prerequisites of |
@brunoffmateus Dependencies should always be documented as explicitly as possible (better yet via automation that fails with a helpful error message). So please do not remove it. It's great that you did what you did. :-) |
@brunoffmateus LGTM but please fix the linter |
@petermetz i believe your concerns are addressed. Can we use the squash and merge functionality (there are 2 PRs), or should they be squashed? |
9b0aa48
to
bb94b91
Compare
🎉 All dependencies have been resolved ! |
@RafaelAPB @brunoffmateus Yes, they were, thank you! I'd still recommend using the "manual" method of rebasing onto main interactively and squash them together that way just to be sure. The github GUI can have negative & unintended side effects sometimes. |
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.
Looks awesome, thank you for this contribution! I've left a few comments / questions :)
packages/cactus-plugin-ccmodel-hephaestus/src/main/typescript/plugin-ccmodel-hephaestus.ts
Outdated
Show resolved
Hide resolved
...hephaestus/src/test/typescript/fabric-contracts/lock-asset/chaincode-typescript/package.json
Outdated
Show resolved
Hide resolved
...hephaestus/src/test/typescript/fabric-contracts/lock-asset/chaincode-typescript/package.json
Outdated
Show resolved
Hide resolved
@outSH: I’m not aware of it. Good to create one in my opinion. It would be nice to have a `custom-check` for it verifying a lot of these fields as well (but I understand that’s a bigger piece of work then just doing a one-off search and replace)
|
bb94b91
to
f9f93f2
Compare
c7d8a74
to
36e7c0e
Compare
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.
@RafaelAPB This one is ready for merge, but before we do please squash the commits together and rebase onto upstream/main
36e7c0e
to
7551bac
Compare
@outSH and @petermetz thanks a lot for the reviews. |
@RafaelAPB My pleasure! :-) |
@RafaelAPB Took a quick look and found a few low hanging fruit fixes (easy to spot bugs that were breaking the build). I'm fixing them but I'll also post some comments here for awareness: The root tsconfig.json file that ties all the packages together for the typescript compiler was missing the hephaestus plugin's reference so the plugin code was not part of the compilation when you did a The indirect way this manifested as an issue was that the build was seemingly good (it compiled successfully) but despite that when you tried to run one of the testcases of the package, Jest would crash with compilation errors within the test case itself. Adding this snippet below to the root tsconfig.json file fixed the root cause but then it highlighted the other issues that were otherwise hidden until now, more on those later. {
"path": "./packages/cactus-plugin-ccmodel-hephaestus/tsconfig.json"
}, |
@RafaelAPB Afterwards, I moved my attention to the compilation issues (see screenshots for full picture).
Command failed: python3 /home/peter/a/cacti-upstream/packages/cactus-plugin-ccmodel-hephaestus/src/main/python/create_model.py /home/peter/a/cacti-upstream/packages/cactus-plugin-ccmodel-hephaestus/src/test/ccLogs/json/hephaestus_log_1734293360776.json
Traceback (most recent call last):
File "/home/peter/a/cacti-upstream/packages/cactus-plugin-ccmodel-hephaestus/src/main/python/create_model.py", line 6, in <module>
import pm4py
ModuleNotFoundError: No module named 'pm4py'
9 | try {
10 | const startTime = new Date();
> 11 | const serializedCCModel = execSync(command).toString("utf-8");
| ^
12 | const finalTime = new Date();
13 | console.log(
14 | `CREATE-MODEL-PM4PY:${finalTime.getTime() - startTime.getTime()}`, |
@RafaelAPB Finally, I'm going to force push now onto your branch with the fixes mentioned above which I'm hoping won't be too confusing and are steps in the right direction. If the CI has the correct setup steps for the python dependencies then it should actually be able to run the tests and have them succeed and if that is the case then I'm good to merge this right after that. If the CI ends up having issues with the tests too then I'll ask that you tweak the CI job to have the python dependencies set up as needed just so that we can see for now at least on the CI passing the tests and I'll make some effort in the future to set up the python dependencies on my dev machine(s) as well. |
Implementation of the paper Hepheastus https://ieeexplore.ieee.org/document/10363680 Authored-by: Bruno Mateus <brumat315@gmail.com> Co-authored-by: Rafael Belchior <rafael.belchior@tecnico.ulisboa.pt> Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com> Signed-off-by: Rafael Belchior <rafael.belchior@tecnico.ulisboa.pt> fix(ccmodel-hepheastus): incorporate PR feedback Authored-by: Bruno Mateus <brumat315@gmail.com> Signed-off-by: Rafael Belchior <rafael.belchior@tecnico.ulisboa.pt> Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
7551bac
to
d2fac8a
Compare
@petermetz, I appreciate the help and your time fixing the issues. I take all responsibility for overlooking those issues. Regarding the Python dependencies, that's a good point. This does not work without that specific dependency, but there is no JS/TS alternative (to the best of my knowledge). I propose that future developments for this plugin can migrate to a JS solution, or we remove it if there is no broader interest in it. |
@petermetz seems we do not have the CI configured for this specific plugin. I'd say I add to the documentation the need for this dependency and (optionally) add the CI configuration for running this package tests (however I'd rather not before it is updated/worked upon). What is your opinion? |
@petermetz I've noticed you already added it. Thanks! |
@RafaelAPB Oh, /facepalm, sorry I didn't even look. We should definitely add a new issue to track adding those CI steps.
@RafaelAPB I'm OK with that schedule, as long as we track the steps to be done via issues (which guarantees that we don't forget/lose track/falls through the cracks/etc.) |
@RafaelAPB I would recommend two ideas for consideration (no idea which one is better or if any of them would work well in practice for now)
|
Thanks for your suggestions @petermetz. We will commit to implement 1. (or to remove the package altogether because as is it breaks at least one of the principles of the project) |
Implementation of the paper Hepheastus https://ieeexplore.ieee.org/document/10363680
Authored-by: Bruno Mateus brumat315@gmail.com
Co-authored-by: Rafael Belchior rafael.belchior@tecnico.ulisboa.pt
Co-authored-by: Peter Somogyvari peter.somogyvari@accenture.com
Signed-off-by: Rafael Belchior rafael.belchior@tecnico.ulisboa.pt
fix(ccmodel-hepheastus): incorporate PR feedback
Authored-by: Bruno Mateus brumat315@gmail.com
Signed-off-by: Rafael Belchior rafael.belchior@tecnico.ulisboa.pt
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.