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

Revive basic federated execution for TypeScript target. #538

Merged
merged 12 commits into from
Sep 28, 2021
Merged

Conversation

hokeun
Copy link
Member

@hokeun hokeun commented Sep 25, 2021

Now we can code-generate Distributed HelloWorld example and run the source and print in TypeScript.

…erate IDs to send neighbor structure to RTI properly.
…ReactionBody() and generateNetworkOutputControlReactionBody() in TSReactionGenerator.
…lin2

# Conflicts:
#	org.lflang/src/org/lflang/generator/ts/TSGenerator.kt
#	org.lflang/src/org/lflang/generator/ts/TSReactionGenerator.kt
….makeCommunication(). This is because, for now, we use TypeScript Buffer type for FederatePortAction for incoming network messages.
… example with instructions to run the federated program with the standalone RTI.
@@ -349,6 +355,13 @@ class TSReactionGenerator(
if (reactor.isFederated) {
generatedReactions = LinkedList<Reaction>()
for (reaction in reactor.reactions) {
// TODO(hokeun): Find a better way to gracefully handle this skipping.
// Do not add reactions created by generateNetworkOutputControlReactionBody
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't the body of the control reactions just be empty for now, instead of skipping them altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think it's cleaner to skip these reactions until we figure out what is the right thing to do, I mean in terms of implementation. We have to handle the special case of generating an empty body for these reactions and I'm concerned that will make the code much more complicated. Please let me know if my concern makes sense.

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 this string matching that is happening here is more prone to breakage than just generating an empty reaction. What if the name of the reaction changed? Are you worried about cluttering the generated code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I'm following "what if the name of the reaction changed". This code doesn't depend on the name of the reaction.

It just uses the string created inside the overridden function, generateNetworkOutputControlReactionBody(). So as long as the TypeScript-specific code generation from generateNetworkOutputControlReactionBody() doesn't change, we're fine.

What I'm worried about is that I may have to have temporary codes in multiple places (i.e., in multiple .kt) and it can become harder to maintain later.

May I ask what benefits we can get by having an empty body rather than skipping the reaction entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, by the way, I'm not sure what generateNetworkOutputControlReactionBody() does. Would this be necessary for all targets? Or it may not be used at all by some targets?

Copy link
Contributor

Choose a reason for hiding this comment

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

These reactions are essential to enable support for distributed cycles. You can sometimes receive PTAGs from the RTI and need these reactions in place to block certain reactions from executing using the input control reactions until you either receive a follow-up TAG or a port absent (via an output control reaction), which in both cases will unblock the input control reactions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Thank you for your explanation! I think we will need to implement that functionality in reactor-ts first before supporting these reactions here.

|}
""".trimMargin()}
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it is better to put these functions in a TSFedGenerator.kt.

Copy link
Member Author

@hokeun hokeun Sep 25, 2021

Choose a reason for hiding this comment

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

These are functions that need to be overridden, so I put them in TSGenerator which inherits GeneratorBase. Do you have any specific suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. You're right. It is unfortunate that these functions should end up in here. Perhaps we should rethink this design altogether and move these functions out of GeneratorBase

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think this is a symptom of our monolithic generator base. I had similar issues in the C++ target, for instance with the methods used for generating after delays.

): String {
return with(PrependOperator) {"""
|// FIXME: For now assume the data is a Buffer, but this is not checked.
|// Replace with ProtoBufs or MessagePack.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just regarding the comment, the SupportedSerializers should be what dictates the serialization. There are both native and proto serializer options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I just noticed SupportedSerializers was added here after I started working on the code and got added here when I pulled the recent master to this branch.

I updated this FIXME to use serializer in c58677c.

By the way, this FIXME was here since I just copied and pasted the code used in the legacy TypeScript federated execution example (which doesn't work for now) here without thinking about it too much: https://github.com/lf-lang/reactor-ts/blob/master/src/example/generated/DistributedLogical/Distributed_dsp.ts

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 now that the TS target is getting attention, we should keep these TS federated functionalities up-to-date with any changes related to federated execution. I will make a mental note to look here whenever I make a change related to federated execution. Adding tests will also help with this matter. We should also probably make changes to the federated execution more collaboratively between all of us going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a heads-up, I am going to make changes to how and where output control reactions are generated. These changes are needed for a correct functionality. I don't expect these changes to affect the TS target that much, but to err on the safe side, we should probably have a meeting about it if you are interested.

Copy link
Member Author

@hokeun hokeun Sep 25, 2021

Choose a reason for hiding this comment

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

Sure, having a meeting sounds good to me! I'd like to learn the overall communication and control flow between the RTI and federates as well. What would be a good time for a meeting for you? Maybe we could invite others who might be interested in this topic as well? For the upcoming three weeks, I'll be mostly available before 6 PM in Pacific time.

timer t(1 sec, 1 sec);
reaction(t) -> message {=
message = root + " " + count++;
console.log(`At time (elapsed) logical tag ${util.getElapsedLogicalTime()}, source sends message: ${message}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting. I don't know how the TS target works, but shouldn't the message be set somewhere using an API? Is = doing the job of a .set?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the way TypeScript works for now is to handle .set in the generated code. Here is an example: https://github.com/lf-lang/reactor-ts/blob/master/src/example/generated/DistributedLogical/Distributed_dsp.ts#L81

I don't think this is ideal, but I'm just using it as is since this is what's available for now.

@@ -0,0 +1,72 @@
/**
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 if this example was turned into a test, it might be less of a headache to keep whatever is already working intact as we make changes to federated execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the great suggestion. I tried to turn this into a test, but realized that we need to make significant changes to the TypeScript code generation and the testing process, to support the federated test for the TypeScript target. I planned out how to enable federated tests in TypeScript in my comment here.

Do you think you could approve this pull request for now? I'm afraid that if the pull request is left for too long, it might be harder to merge later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I would say that this is a great improvement. I just didn't want these updates to become outdated as soon as something is changed in the federated infrastructure. We can manually attempt to keep it up until these issues are resolved.

…nificant changes to the code generation process (to generate a shell script to run RTI and federates) and the testing infra (execute() in TestBase.java to run the shell script instead of .js file).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants