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

Restructuring of mechanism for compiling federated programs #1221

Merged
merged 379 commits into from
Jan 20, 2023
Merged

Conversation

lhstrh
Copy link
Member

@lhstrh lhstrh commented Jun 10, 2022

Edit. Refactor logic related to federated execution out of GeneratorBase, CGenerator, PythonGenerator, and TSGenerator into a new org.lflang.federated.FedGenerator.

  • Move analyzeFederates() out of GeneratorBase into FedGenerator (@Soroosh129)
  • Have FedGenerator create separate .lf files for each federate (see [Federated Execution] Draft Proposal: Revamping the code generation process for federations #1212) (@Soroosh129)
  • TS (@lhstrh, @hokeun)
    - Note: For the TS target, the code for these interfaces is already in the TSGenerator (e.g., here)
    • Implement "executable" preambles (using a syntax such as init (suggested by @cmnrd)) for each target to take care of pre-start tag initializations:
      • C target (@billy-bao)
      • Create a mechanism that can convert TargetConfig to TargetDecl
      • Note: For the C and TS targets, the code for pre-start tag initialization is in their respective code generators, usually surrounded by if (isFederated) checks. Implementing the executable preamble would mostly require moving these blocks to the federated generator.
  • Output interface instead of using the @interface label (@lhstrh) Add internal dependency information to reactions
  • Have FedGenerator call each code generator depending on the target of each generated .lf file (@billy-bao)
  • Fix the --clean flag, which does not work for federated programs because they are located in a separate fed-gen directory (@axmmisaka)
  • Fix Docker support for C (@lhstrh)
  • Fix Docker support for TypeScript (@lhstrh)
  • Fix exception throw when bin directory already exists:
Exception in thread "pool-1-thread-2" org.eclipse.xtext.util.RuntimeIOException: Failed to create the directory /home/marten/lingua-franca-master/git/lingua-franca/test/C/fed-gen/DistributedCount/bin
	at org.lflang.util.FileUtil.createDirectoryIfDoesNotExist(FileUtil.java:486)
	at org.lflang.generator.c.CGenerator.doGenerate(CGenerator.java:468)
	at org.lflang.generator.LFGenerator.doGenerate(LFGenerator.java:181)
	at org.lflang.federated.generator.FedGenerator.lambda$compileFederates$0(FedGenerator.java:246)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
  • Re-enable LetInferenceTests

@lhstrh lhstrh changed the title Add capability to output individual federates as standalone LF code Add capability to generate LF code and create a CLI program for formatting Jun 12, 2022
@petervdonovan petervdonovan force-pushed the fed-gen branch 2 times, most recently from c1fa046 to b15c141 Compare June 12, 2022 23:04
@petervdonovan
Copy link
Collaborator

Grr, GitHub does not allow for the source branch of a PR to be changed, so I will have to move the title and comments used here to #1227.

@petervdonovan petervdonovan changed the title Add capability to generate LF code and create a CLI program for formatting Add capability to output individual federates as standalone LF code Jun 13, 2022
@Soroosh129 Soroosh129 changed the base branch from master to pretty-printer June 13, 2022 17:09
Copy link
Member

@hokeun hokeun left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of the TS generator!

I left a couple of comments to understand this change better.

Also, I have one more question:

I tried to Lfc-compile test/TypeScript/src/federated/HelloDistributed.lf, but I got an error like this:

******** Using 2 threads to compile the program.
lfc: error: Mixed triggers and effects from different federates. This is not permitted
 --> test/TypeScript/src/federated/HelloDistributed.lf:44:5
   |
43 | federated reactor HelloDistributed at localhost {
   | >>>>>>>>>>>>>>
44 |     reaction(startup) {=
45 |         console.log("Printing something in top-level federated reactor.");
46 |     =}
   | <<<<<< Mixed triggers and effects from different federates. This is not permitted
47 |     s = new Source();      // Reactor s is in federate Source

lfc: fatal error: Aborting due to previous error

FAILURE: Build failed with an exception.

It doesn't look related to the fed-gen changes. Are we supposed to get such errors?

@Soroosh129
Copy link
Contributor

Soroosh129 commented Jul 24, 2022

@hokeun:

I tried to Lfc-compile test/TypeScript/src/federated/HelloDistributed.lf, but I got an error like this:

You are using zero-delay logical connections between federates. This currently does not work because the causality interface (@interface, see the original comment on this PR) is not implemented yet. Adding an after delay should circumvent this problem for now.

@Soroosh129
Copy link
Contributor

I'm a bit stuck at the moment in my attempt to remove currentFederate from the CGenerator.

Specifically, there are multiple places in the CGenerator where running counts (that are class attributes) get incremented by currentFederate.numRuntimeInstances(...). I have no idea what these are for and how I might be able to remove them. @edwardalee would you happen to remember what each of these running counts are for? I think I need your help to get rid of them.

Here are some examples where this pattern is used: 1 2 3 4 5 6 7.

@edwardalee
Copy link
Collaborator

edwardalee commented Jul 24, 2022

I'm a bit stuck at the moment in my attempt to remove currentFederate from the CGenerator.

Specifically, there are multiple places in the CGenerator where running counts (that are class attributes) get incremented by currentFederate.numRuntimeInstances(...). I have no idea what these are for and how I might be able to remove them. @edwardalee would you happen to remember what each of these running counts are for? I think I need your help to get rid of them.

These counts are used to determine the sizes of statically allocated arrays, for example, the array of startup reactions that need to be invoked at startup.

The issue here is that one instance of ReactorInstance may represent some number of runtime instances of the reactor. The number depends on whether the reactor is instantiated as a bank, whether any of its containers is instantiated as a bank, and whether it's top-level container is in the federate. The latter condition is particularly subtle because if the top-level container is a bank, then the width of the bank is irrelevant and should be treated as 1 because only one bank member will be in the federate.

I presume that the latter subtlety will go away if you are code generating a .lf file for each federate because then no top-level container will be a bank, right? Also, the check for whether the ReactorInstance is in the federate can probably be eliminated because this will not be called on any reactor that is not in the federate. If I'm right about these two, then you can probably replace currentFederate.numRuntimeInstances(reactor) with reactor.getTotalWidth(0). In fact, you could update ReactorInstance.getTotalWidth() to not take a depth argument because I suspect it will only ever be called with depth == 0.

@Soroosh129
Copy link
Contributor

[...] no top-level container will be a bank, right?

Right. At least that's what I'm aiming for.

In fact, you could update ReactorInstance.getTotalWidth() to not take a depth argument because I suspect it will only ever be called with depth == 0.

Thank you! I applied your solution in c75d02a. Luckily, there is already a version of getTotalWidth that takes not argument (and passes 0 as the depth to ReactorInstance.getTotalWidth(int depth)).

Copy link
Member

@hokeun hokeun left a comment

Choose a reason for hiding this comment

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

I'll take another look at FIXMEs assigned to me early this week!

@hokeun hokeun self-requested a review July 25, 2022 04:05
@Soroosh129 Soroosh129 changed the base branch from pretty-printer to preserve-comments July 26, 2022 07:18
Base automatically changed from preserve-comments to pretty-printer July 27, 2022 21:30
@lhstrh lhstrh changed the title Add capability to output individual federates as standalone LF code Capability to output individual federates as standalone LF code Aug 4, 2022
@cmnrd
Copy link
Collaborator

cmnrd commented Jan 18, 2023

I just noticed that there are some more weird issues with the Docker support. It seems that docker files are now generated for all federated programs, no matter if they have docker support enabled or not. Also, even containerized federated programs produce a then useless launch script.

@lhstrh
Copy link
Member Author

lhstrh commented Jan 18, 2023

Error code 139 observed in this CI run suggests a segfault due to wrong format specifiers in a printf. I have seen a bunch of warnings in the C compilation output of both federates and the RTI, but I'm not sure if they could be the source of the problem. Tagging folks who might be able to help: @erlingrj, @edwardalee, @petervdonovan.

@lhstrh
Copy link
Member Author

lhstrh commented Jan 19, 2023

I notice that if I run the same federated containerized test in rapid succession, then after a few tries I inevitably run into this:

DistributedCountContainerized-rti  | WARNING: RTI failed to accept the socket. Resource temporarily unavailable. Trying again.
DistributedCountContainerized-p    | DEBUG: _lf_create_token: Allocated memory for token: 0x7f7b570ae930
DistributedCountContainerized-p    | Federate 1: LOG: Connecting to the RTI.
DistributedCountContainerized-p    | Federate 1: Failed to connect to RTI on port 15045. Trying 15046.
DistributedCountContainerized-p    | Federate 1: Could not connect to RTI at rti. Will try again every 2 seconds.
DistributedCountContainerized-p    | Federate 1: Failed to connect to RTI on port 15046. Trying 15047.
DistributedCountContainerized-p    | Federate 1: Could not connect to RTI at rti. Will try again every 2 seconds.
DistributedCountContainerized-p    | Federate 1: Failed to connect to RTI on port 15047. Trying 15048.
DistributedCountContainerized-p    | Federate 1: Could not connect to RTI at rti. Will try again every 2 seconds.
DistributedCountContainerized-p    | Federate 1: Failed to connect to RTI on port 15048. Trying 15049.
DistributedCountContainerized-p    | Federate 1: Could not connect to RTI at rti. Will try again every 2 seconds.
DistributedCountContainerized-p    | Federate 1: Failed to connect to RTI on port 15049. Trying 15050.
DistributedCountContainerized-p    | Federate 1: Could not connect to RTI at rti. Will try again every 2 seconds.
DistributedCountContainerized-p    | Federate 1: Failed to connect to RTI on port 15050. Trying 15051.
DistributedCountContainerized-p    | Federate 1: Could not connect to RTI at rti. Will try again every 2 seconds.
DistributedCountContainerized-rti  | WARNING: RTI failed to accept the socket. Resource temporarily unavailable. Trying again.

This log suggests to me that the problem is in the RTI, not with the federates. This brings me to another question: which version of the RTI is being used here? The RTI doesn't have any version numbers, so I guess nobody can answer this question, but where does the image come from? The docker-compose.yml file has an entry image: lflang/rti:rti which suggests that an image has been published somewhere. By whom? How do we update it? @housengw do you happen to know anything about this?

@lhstrh
Copy link
Member Author

lhstrh commented Jan 19, 2023

OK, it looks like the docker image was published by @housengw: https://hub.docker.com/u/housengw, ten months ago.

@edwardalee
Copy link
Collaborator

Error code 139 observed in this CI run suggests a segfault due to wrong format specifiers in a printf. I have seen a bunch of warnings in the C compilation output of both federates and the RTI, but I'm not sure if they could be the source of the problem. Tagging folks who might be able to help: @erlingrj, @edwardalee, @petervdonovan.

I put in quite a bit of effort to purge warnings, but never got to the RTI or federated code. Help would be welcome.

In lf_tag_64_32.h it says:

// Define PRINTF_TIME and PRINTF_MICROSTEP, which are the printf
// codes (like the d in %d to print an int) for time and microsteps.
// To use these, specify the printf as follows:
//     printf("Time: " PRINTF_TIME "\n", time_value);
// On most platforms, time is an signed 64-bit number (int64_t) and
// the microstep is an unsigned 32-bit number (uint32_t).
// Sadly, in C, there is no portable to print such numbers using
// printf without getting a warning on some platforms.
// On each platform, the code for printf if given by the macros
// PRId64 and PRIu32 defined in inttypes.h.  Hence, here, we import
// inttypes.h, then define PRINTF_TIME and PRINTF_MICROSTEP.
// If you are targeting a platform that uses some other type
// for time and microsteps, you can simply define
// PRINTF_TIME and PRINTF_MICROSTEP directly in the same file that
// defines the types _instant_t, _interval_t, and _microstep_t.
#include <inttypes.h>
#define PRINTF_TIME "%" PRId64
#define PRINTF_MICROSTEP "%" PRIu32

// For convenience, the following string can be inserted in a printf
// format for printing both time and microstep as follows:
//     printf("Tag is " PRINTF_TAG "\n", time_value, microstep);
#define PRINTF_TAG "(" PRINTF_TIME ", " PRINTF_MICROSTEP ")"

Copy link
Member Author

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This has gotten to a point where I think it is ready to merge, modulo whatever errors might still come up in CI.

@lhstrh lhstrh changed the title Capability to output individual federates as standalone LF code Restructuring of mechanism for compiling federated programs Jan 20, 2023
@lhstrh lhstrh marked this pull request as ready for review January 20, 2023 01:36
@lhstrh lhstrh merged commit 77baf9e into master Jan 20, 2023
@lhstrh lhstrh deleted the fed-gen branch January 20, 2023 08:19
lhstrh added a commit to lf-lang/reactor-c that referenced this pull request Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.