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

Use CallFnAfter instead of CallOnConditionKeep #3841

Merged
merged 2 commits into from
Oct 4, 2024
Merged

Conversation

ritvikrao
Copy link
Contributor

In order to get recent failing test cases to pass so we can merge PRs, I changed the periodic test to use CcdCallFnAfter, not CcdCallOnConditionKeep, which means the test will pass regardless of the value of CkWallTimer in the mainchare of periodic.C.

@lvkale
Copy link
Contributor

lvkale commented Oct 3, 2024

This is fine as a workaround. And ok since its not a critical issue. But I think we should fix the "round number" approximation.
Secondly, we should examine what in the earlier PR triggered this.

@lvkale
Copy link
Contributor

lvkale commented Oct 3, 2024

Also : I am pasting your *=(Ritvik's) explanation from slack here:
I took a closer look and this code can test one of 2 things, CcdCallFnAfter and CcdCallOnConditionKeep. if using CcdCallFnAfter, there’s no error regardless of what the first timer is set as, since when i print the timer on each pe, those times are always later than whatever the timer recorded on the main chare (call after works by taking the wall timer and then setting a callback from that point, so there would never be an error). the problem is with CcdCallOnConditionKeep (which the github tests run on). while CcdCallFnAfter requires you to repeatedly set a callback for 1 second after the current wall timer, CcdCallOnConditionKeep calls a specified function (passed in as an argument) every time a condition is met. in periodic.C, that condition is the macro CcdPERIODIC_1s, meaning that every time the wall timer is equal to a whole number (all zeros after the decimal), it’s called. in short, if the initial wall timer is at 0.42, CcdCallFnAfter will finish at 1.45, 2.48, and 3.51 (due to latencies), while CcdCallOnConditionKeep triggers at 1.1, 2.1, and 3.1 (also due to latencies). this means that CcdCallFnAfter can’t fail but CcdCallOnConditionKeep could if the first wall timer is too late.

@stwhite91
Copy link
Collaborator

It would be better to either 1) fix the test to work for CcdCallOnConditionKeep, or 2) remove CcdCallOnConditionKeep from the test entirely. Leaving it commented out and known to be broken is bad and may confuse someone later looking for an example of how CcdCallOnConditionKeep works.

@lvkale
Copy link
Contributor

lvkale commented Oct 3, 2024

As to this test itself is concerned, I am ok with option 2 that Sam listed. (remove CcdCallOnCondition) and all the associated ifdefs). Its bogus to base it on the "has a bunch of 0's after decimal" to determine close-enough-to-one. And also to assume that startup will be close to 0.
OTOH: whatever caused the the start time to be 0.4+ secs on pe 0 is worth investigating. Maybe open another issue following up on Tom's PR (or examine and fix it now).

@ritvikrao
Copy link
Contributor Author

I pushed a fix to make this work for both ConditionKeep and CallAfter. To be clear, there are no problems with either of these methods, and they work as expected. The issue is that ConditionKeep triggers a function every second starting from 0 (so 1.0, 2.0, 3.0, etc), instead of starting from the moment ConditionKeep is called, which differs from how CallAfter works. I would argue that the original version of the test doesn't make sense for ConditionKeep, since it assumes that CkWallTimer starts at 0. So I just added an ifdef saying that if we're using ConditionKeep, set the start time to 0, not CkWallTimer.

@lvkale
Copy link
Contributor

lvkale commented Oct 3, 2024

ok. I like that and is correct to the intent of the test. (Which may be testing for some really rare malady in timers.. but ok).
But we do need to investigate what was changed by Tom's PR separately.

@ritvikrao
Copy link
Contributor Author

ritvikrao commented Oct 3, 2024

Tom hasn't had a recently merged PR though.

@ritvikrao ritvikrao merged commit 36890a0 into main Oct 4, 2024
23 checks passed
@ritvikrao ritvikrao deleted the ritvik/periodic_test branch October 4, 2024 15:07
@ritvikrao ritvikrao restored the ritvik/periodic_test branch October 4, 2024 17:17
ritvikrao added a commit that referenced this pull request Oct 4, 2024
@ritvikrao ritvikrao mentioned this pull request Oct 4, 2024
@ritvikrao ritvikrao deleted the ritvik/periodic_test branch October 4, 2024 17:28
ritvikrao added a commit that referenced this pull request Oct 4, 2024
* Revert "Use CallFnAfter instead of CallOnConditionKeep (#3841)"

This reverts commit 36890a0.

* ifdef to if
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.

4 participants