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

Test Scheduler by the state progress of each RID #1920

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

Deepskyhunter
Copy link
Contributor

@Deepskyhunter Deepskyhunter commented Jun 21, 2022

ARTIQ Pull Request

Description of Changes

Add SchedulerMonitor
use it to record status change and compare with expect_status
use process_mod to update the schedulers in SchedulerMonitor

await wait_until(rid, <"arrive"/"leave">, status) to operate after certain condition is matched

Use the following method for assertion
get_exp_order(status): the order of exp enter that status
get_status_order(rid): the order of status that the experiment go through

Related Issue #1888

Type of Changes

Type
🐛 Bug fix

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

  • Use correct spelling and grammar.
  • Close/update issues.

Code Changes

Git Logistics

  • Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
  • Write short & meaningful commit messages. Review each commit for messages (git show). Format:
    topic: description. < 50 characters total.
    
    Longer description. < 70 characters per line
    

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

Mark cases that has alternative at alternative_case.
Include the alternative case in the corresponding position in
expect[] with list.
Consider alternative case during assertEqual
@sbourdeauducq
Copy link
Member

That's pretty cryptic.
What about using coroutines to interpret and check the progression of each RID?
Why was the approach in your previous PR seemingly completely abandoned?

@Deepskyhunter
Copy link
Contributor Author

Why was the approach in your previous PR seemingly completely abandoned?

It seems that the previous approach ignore the intermediate state completely.

Deepskyhunter added 3 commits June 22, 2022 10:11
Add parameter due_date to  _get_basic_steps() for more application
Create expectRID for each experiment by using _get_basic_steps()
Put expectRIDs into expect
Create last_state_RID for checking
First identify the RID
Then check the state of the corresponding RID
Last check the state of each RID when one of them passed all
state
Change _get_basic_steps parameters' order to common order
@Deepskyhunter Deepskyhunter changed the title Add alternative to test case Test Scheduler by the state progress of each RID Jun 22, 2022
Deepskyhunter added 2 commits June 22, 2022 14:25
Previous change in parameters' order may result in different input
of existing code.
Put due_date at the back to avoid it.
Update the code that used this function
Comment on lines 169 to 172
if type(mod["key"]) is int:
rid = mod["key"]
else:
rid = mod["path"][0]
Copy link
Member

Choose a reason for hiding this comment

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

What is that doing? There should at least be an explanatory comment in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is for finding the RID of the mod

Copy link
Member

Choose a reason for hiding this comment

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

I had guessed as much, but why is it done that way?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to look at mod["action"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I have separated the expect case into 3 for each RID, then monitor which state does it go through.
This way can know what they are up to without a conflict of slight time delay.

Deepskyhunter added 4 commits June 23, 2022 09:58
Use schedule to record changes from mod.
Mark down the order of rid which status changes from pending to
preparing in pending_to_preparing.
Two RID would change status at the same time when one of
them entering done state (prepare_done and run_done).
So once any of them enter that state, checking in next
notify would be skipped.
Use it in test_pending_priority
check_status to see the update of schedulers is expected or not
Deepskyhunter added 2 commits July 13, 2022 16:18
@@ -82,6 +85,59 @@ def get(self):
self._next_rid += 1
return rid

class SchedulerMonitor():
Copy link
Member

Choose a reason for hiding this comment

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

Remove ()

Comment on lines 114 to 115
self.exp_flow[key].append(time())
self.exp_flow[key].append(current_status)
Copy link
Member

Choose a reason for hiding this comment

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

No. Structure it.

Deepskyhunter added 11 commits July 18, 2022 14:44
Target a particular status of a rid and set the asyncio.Event()
It has been replaced by basic_flow
asyncio.wait to wait either one rid leave pending stage
Update wait_until and record in monitor for successful callback
from leave stage
status_records: Information are stored but never used. Plus it
can be computed from exp_flow afterward
get_time functions are never used.
It was introduced as a condition to end scheduler which is no
longer necessary
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.

2 participants