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

Support modal models in Python #1009

Merged
merged 41 commits into from
Mar 15, 2022
Merged

Support modal models in Python #1009

merged 41 commits into from
Mar 15, 2022

Conversation

edwardalee
Copy link
Collaborator

This is a first step at supporting modal models in Python. We got one test working but ConvertCaseTest.lf gives different results, so this needs more work. This PR depends on branches by the same name (python-modal-models) in reactor-c and reactor-c-py.

@edwardalee edwardalee requested a review from a-sr March 8, 2022 18:52
@edwardalee edwardalee changed the title First step at supporting modal models in Python Support modal models in Python Mar 9, 2022
@@ -31,18 +31,18 @@ public static void generateDeclarations(
if (!allModes.isEmpty()) {
// Reactor's mode instances and its state.
body.pr(String.join("\n",
"reactor_mode_t _lf__modes["+reactor.getModes().size()+"];",
"reactor_mode_state_t _lf__mode_state;"
Copy link
Collaborator

@a-sr a-sr Mar 14, 2022

Choose a reason for hiding this comment

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

You moved the reactor_mode_state_t _lf__mode_state into the self_base_t struct. If I understand this correctly, now every reactor has a mode state struct (as soon as any modes are present in general) whether it has modes or not. I did not do that to save memory. Is there a benefit in doing it this way that I do not see or does it simply not matter that much?

Copy link
Contributor

@Soroosh129 Soroosh129 Mar 14, 2022

Choose a reason for hiding this comment

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

Is there a benefit in doing it this way?

We needed this because we wanted to call _LF_ SET_MODE in the .set method of the mode type in the Python target. _LF_ SET_MODE expects a self pointer to a self struct that has a reactor_mode_state_t _lf__mode_state; member. Since self structs are typed, we needed to move it to the base so that it works in the type-agnostic .set method implementation.

The idea is that we want to re-use _LF_ SET_MODE so that any changes in the C runtime to the functionality of this macro is also automatically reflected in the Python runtime.

The implementation of the .set method is here in case you are interested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. Thanks.

Copy link
Collaborator

@a-sr a-sr left a comment

Choose a reason for hiding this comment

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

Looks good to me. I see no reason not to merge it.

@Soroosh129
Copy link
Contributor

Soroosh129 commented Mar 14, 2022

@housengw Could you please have a look at the conflicts with master? The conflicts seem to primarily be in the imports, but it would be great if you could see if anything needs attention since you are leading the refactoring effort.

@housengw
Copy link
Contributor

will do.

@housengw housengw marked this pull request as ready for review March 14, 2022 17:55
@housengw housengw requested a review from Soroosh129 March 15, 2022 06:45
Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @housengw for fixing the merge issues.

@Soroosh129 Soroosh129 merged commit 85fb426 into master Mar 15, 2022
@Soroosh129 Soroosh129 deleted the python-modal-models branch March 15, 2022 13:29
@lhstrh lhstrh added the enhancement Enhancement of existing feature label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants