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

Update level assignment algorithm #1865

Closed
wants to merge 81 commits into from
Closed

Conversation

petervdonovan
Copy link
Collaborator

@petervdonovan petervdonovan commented Jun 24, 2023

This includes changes required in order to implement TPO.

Status update 6/24

The basic idea is almost implemented, but still only 43/57 tests pass (previously it was 41/57). I think that most of the tests with cycles are still deadlocking because the output reactions that send "port absent" messages are not being enqueued. This should be an easy fix. Also the method of assigning the TPO is not entirely correct yet, for uninteresting software engineering reasons.

Status update 6/26

Most of the tests are passing now. 53 ish out of 57 depending on how you count, BUT some of the failures are nondeterministic, so probably there are others that will also sometimes fail.

  • FeedbackDelay and FeedbackDelaySimple are failing due to falsely detecting cycles when there is actually a 1 microstep after delay
  • DecentralizedP2PComm is failing with wrong functional behavior (STP violation in the generalized sense, I believe). This failed in the second-to-last run but not the last one. It may be nondeterministic, so this isn't surprising.
  • DistributedLoopedPhysicalActionDecentralized failed with an error that I could not reproduce locally, so the situation there is probably similar.

TODOs

Whoever chooses to pick up any of these tasks should write their name next to the task to make sure there is no duplicate work.

  • Use a less hacky mechanism for marking ports that receive from delayed connections as not part of the MLAA calculation (@petervdonovan)
  • Find dead/unnecessary code in the code generator and delete it (split this into sub-tasks as needed) (the CodeCov reports may help with this)
  • Pass the other C tests besides the C federated tests
  • Ensure IntelliJ does not emit warnings related to the code that is touched by this and the other related PRs
  • Get this branch up to date with master (@petervdonovan)
  • Check that this also works for the Python target (@petervdonovan)
  • Factor C-specific code out of FedASTUtils and into CExtension or CExtensionUtils (@petervdonovan)
  • Get all tests in reactor-c to pass (@petervdonovan)
  • Get the TypeScript runtime to be compatible with this branch (@byeong-gil)
  • Fix the broken C tests and check that they still pass
  • Fix the broken Python tests and check that they still pass
  • Check that all code added has JavaDoc

Non-required TODOs

  • Use library reactors as the network senders and receivers instead of generated reactors (@petervdonovan)
  • Reduce the complex hidden dependencies between different functions through their interaction with the state of mutable objects
  • Do not add the special reactors for indexing into multiports
  • Find dead/unnecessary code in the C runtime and delete it

arengarajan99 and others added 30 commits March 29, 2023 20:40
I do not want to stop running tests when one category fails. I would
also like to experiment with increasing the amount of parallelism.
@petervdonovan
Copy link
Collaborator Author

Apparently the code does not currently expect to receive "port absent" messages after it has already assumed a port absent and labeled its status as known to be absent at the current tag (why not?). Also, FeedbackDelay now deadlocks for unknown reason.

@petervdonovan
Copy link
Collaborator Author

The other day @lhstrh asked about the status of this PR. The federated C tests finally passed in this branch (:tada:), so at least we know that it will be possible to get this to work within a reasonable amount of time. However there is a lot of cleanup left to do (maybe a few weeks) before this is good enough to merge into master.

@lhstrh
Copy link
Member

lhstrh commented Jul 6, 2023

That is fantastic news! Perhaps you could compile a list of todos for the cleanup so that others could help out with some of the items?

There is also a change to ensure that the reactors in the generated LF
code correspond to distinct reactor classes. If I am not mistaken this
fixes a scalability issue.
@petervdonovan petervdonovan changed the base branch from top-level-multiports to master July 11, 2023 19:33
@petervdonovan
Copy link
Collaborator Author

Today I mentioned that in this branch there is an STP violation in LoopDistributedDecentralized that should not occur. Just now I checked and found that this error also occurs in master, even when the STP offset and deadline are both very large (100 msec). Probably fixing this in this branch should not be required in order to merge this into master.

@petervdonovan
Copy link
Collaborator Author

Work on this is being moved back to the remove-unordered-reactions branch.

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.

3 participants