Fix for ASTUtils.width() returnning -1 even when the width can be inferred from connections
#1287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edwardalee
reviewed
Jul 9, 2022
There was a problem hiding this comment.
I'm not sure about this change. The failure in Cpp is worrisome, but I can't read the CI output so I don't know the failure actually is. The Cpp tests log output has an absurd number of printf outputs... Are these all needed? Anyway, I search the logs for "failed" and "error" and found nothing, but the CI reports that the test failed.
I think that width == 0 in delays means that the width need to be inferred from the context. So I'm not sure that simply ignoring it is the right thing to do. But again, I can't find anywhere that this field maxWidth is used (Eclipse tells me there are no references to it), so don't know what the implications are.
|
Thank you for your comments, @edwardalee!
I think you meant this error in cpp-benchmark-tests/run: Here is the error message I tracked down: To me, it looks like the root cause of the error above might not be related to this change, but I'm not 100% sure yet. Do you think you could recommend a point of contact to reach out to regarding this error? Maybe @cmnrd?
I'm not sure either... But without this change, we get an error in existing tests when
You're right. |
hokeun
added a commit
that referenced
this pull request
Jul 11, 2022
…g logic for inferring width from connections into width() function of ASTUtils.java. (Related PR: #1287).
…g logic for inferring width from connections into width() function of ASTUtils.java.
hokeun
changed the title
|
@edwardalee, I fixed this issue in a different way, by inferring the width from connections when it's possible. I believe this fixes the fundamental issue. Do you think you could take another look when you get a chance? Thanks! |
|
I am not sure which CI run you are talking about. If you provide me with a link, I am happy to take a look. The line |
|
Oh, I found the log. Here is the relavant bit: Indeed there is a lot of text printed. For some reason some benchmarks don't suppress info messages. Also oddly for some command many empty lines are printed. I will take a look. |
edwardalee
approved these changes
Jul 12, 2022
There was a problem hiding this comment.
Ah, OK, this looks like a better way to solve the problem, and the output @cmnrd found indicates that indeed the test failure was related to the change. The relevant line was this one:
2022-07-11T21:20:54.1375165Z [2022-07-11 21:20:54,136][/home/runner/work/lingua-franca/lingua-franca/lf/bin/lfc][INFO] - org.lflang.generator.InvalidSourceException: Multiple ports with variable width on a connection.
I need someone to show me how to read the "Checks" tab for a PR. I was unable to find any meaningful output at all in that tab. Clearly I am looking in the wrong place.
|
In the meantime I pushed a fix to the benchmarks repo which should reduce the noise in the benchmark logs (in particular the C++ logs). (See lf-lang/benchmarks-lingua-franca#27). In this particular case, the log for the c++ benchmarks here did not contain the problem. So probably this is why you did not find it. Note the last line stating: I clicked in the gear in the top-right and then on "View raw logs" to see the full log. Then I searched the raw log. With the fix in lf-lang/benchmarks-lingua-franca#27, the log should get much smaller and you should be able to search directly in GitHub's log view. Note that searching only works properly with the built-in search tool and not with you browser's search. |
|
Thank you, @edwardalee and @cmnrd, for your reviews and great discussion!
I think you're right. The previous version of the fix did cause that error to happen, so I carefully crafted the second version to avoid changes that can affect currently working LF programs. Luckily, the modified version of the fix did not affect the existing tests.
Sounds great. This should help us look into the error logs. Thanks! |
lhstrh
changed the title
ASTUtils.width() returnning -1 even when the width can be inferred from connections
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to fix an error found while working on #1275.
Weirdly, adding
createMainReactorInstance()to TypeScript code generator breaks following existing tests.Below is an example raw error message from the broken tests' runLfc:
Specifically, when the
widthbecomes 0,RuntimeRange<PortInstance> srcbecomes null.