[SFN] basic version of stepfunctions v2#7464
Conversation
…est suite updates, lintings
thrau
left a comment
There was a problem hiding this comment.
it's really great to see our own implementation of stepfunctions taking shape! 👏 looking forward to learning more about the concepts introduced in the PR
@MEPalma, could you add a bit of explanation for people who aren't too familiar with stepfunctions and antrl. how is antlr used in this, is it a build or a runtime dependency? is there any type of architecture diagram that gives me a basic understanding of how this works? i'd love to test it a bit to learn more :-)
|
FYI: The failing test seemed to be a bit flaky. It went green after I retriggered the build. |
dominikschubert
left a comment
There was a problem hiding this comment.
Added a few comments, most of those were discussed in our last sync 👍
There was a problem hiding this comment.
Absolutely amazing and massive set of changes @MEPalma ! 🚀 🚀
As we briefly discussed (also with @dominikschubert ), the ANTLR based lexer/parser implementation is really sophisticated - kudos for pulling off such an advanced implementation. (Maybe you've not chosen the easiest route with this approach, ;) but still impressive to see how you've come up with this e2e implementation!).
Added a few smaller comments and suggestions, but nothing that should really block the initial merge. This is a great foundation that will allow us to iterate on the new SFn provider - can't wait to see it in action and replacing the old provider (which effectively we can no longer upgrade as it has become so bloated in the meantime). Great job! 💪 🥳
| @@ -0,0 +1,21 @@ | |||
| import threading | |||
There was a problem hiding this comment.
minor: Wondering if we could create a cleaner separation between the ASL language artifacts (incl. grammar, lexer, parser, and generated code), and the execution runtime. Currently, the concepts are a bit interwoven in the code structure.
It seems like a first quick win could be to pull out the stepfunctions.asl.eval module out into a stepfunctions.runtime module - which would more cleanly separate the concepts.
There was a problem hiding this comment.
Excellent point. In ext we have an iteration of this project which separated components into Components and Evaluation. This seems similar to the idea you mention here.
Evaluation component used to decorate Components, thereby separating the two aspects. As the Components abstraction appeared to duplicate ANTLR's 'nodes' abstraction, and given the added amount of files it involved (2 per component) and verbose code (constructors mainly), the decision was made to have EvalComponents extend Components. Another minor aspect to address in that setup was how some Components do not require an evaluation logic, but are members of other Components.
I still second the idea of having this separation, but I see how this would require a more careful design first.
With the exception of utils files all in stepfunctinos.asl is eval/runtime code.
whummer
left a comment
There was a problem hiding this comment.
Thanks for addressing all the comments. 🚀 LGTM, but will let @dominikschubert @giograno do the final sign-off.. 👍
dominikschubert
left a comment
There was a problem hiding this comment.
Changes since last review LGTM
All good for an initial merge from my side 👍
Introduces initial efforts on stepfunctions
v2provider. Enable withPROVIDER_OVERRIDE_STEPFUNCTIONS=v2. Please refer to knowledge-base for an overview of these changes.