HWP Emulation and PID agent lockless restructure #606

jlashner · 2024-01-04T20:16:18Z

This PR begins to implement an emulation framework for the HWP, to be used for testing HWP agent operation. Also restructures the HWP_PID to use the lockless format.

Description

During the dev call, and while testing out HWP supervisor status at the site, there are a number of things that we still need to update in many of the HWP agents before we can reliably use them to observe. Due to the limited access to HWP hardware, a relatively realistic HWP emulator will be extremely helpful for developing changes, and testing them before testing on real hardware. This PR begins to implement this emulation, and I have tested it running against PID and PMX agents on my local development setup.

The data being returned from the emulator isn't perfect (especially since the PID interface requires some more reverse engineering), but it is good enough such that agent tasks are able to run successfully.

In testing, I was finding many instances in which the PID agent could not successfully run operations due to the locking structure, so I refactored that agent to have the same structure as the new timeout-lockless agents, and tested with the emulator.

This PR contains the following changes:

Start of the HWP emulation framework
PID lockless restructure
some logging modifications to the DeviceEmulator class to make it more usable by general emulators like this

How Has This Been Tested?

Tested by running with the HWP PMX and PID agents locally.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

ykyohei

The lockless restructure of hwp-pid agent looks good to me.
I really want to make HWP control more robust, so I really appreciate this work.

BrianJKoopman

Thanks for PRing this, and sorry it took me a bit to get to. Is the intention here to eventually run integration tests for the simulator with an instance of this stood up? Do you have any examples of these tests yet?

The biggest thing to me is the logging issues pointed out in the comment on device_emulator.py. Other comments are relatively minor.

simulators/hwp/hwp_emulator.py

socs/agents/hwp_pid/agent.py

BrianJKoopman · 2024-01-10T23:19:22Z

socs/agents/hwp_pid/agent.py

+    if args.mode is not None:
+        agent.log.warn("--mode agrument is deprecated.")


Since this is true, we should remove it from the example config block in the docs.

mhasself · 2024-01-11T14:32:54Z

socs/agents/hwp_pid/agent.py

-        return True, 'Reversing Direction'
+        action = Actions.TuneStop(**params)
+        self.action_queue.put(action)
+        session.data = yield action.deferred


Reminder to keep session.data as a dict. Maybe standardize on session.data['output'] = ... or something like that?

Also -- what happens here if there's an errback? Do you need to catch that?

If there's an errback, the yield will just throw the error. I find this to be the easiest way to find and debug errors in ocs operations anyway, so I don't see the need to catch and handle manually.

jlashner · 2024-01-17T16:47:39Z

Hi Brian, thanks for the comments, I think I addressed or responded to all of them.

My goal with this is not necessarily to use this in tests, though it may be possible. Mainly the purpose of this is to allow myself and other HWP developers to run HWP agents for the PID, PMX, supervisor, etc. on their local systems, which is very helpful for developing new features and testing behavior.

BrianJKoopman

The recent changes look good to me. There's just the failing tests and docs builds now.

Merging in the most recent main should fix the docs builds. The tests will need fixing before we can merge though.

for more information, see https://pre-commit.ci

jlashner · 2024-01-29T22:54:46Z

Hi Brian, so I wasn't originally intending to use the emulator for tests, but when trying to fix the tests for the PID agent restructure, it ended up being much easier to use the emulator, since the PID responses are difficult to write out manually.

With the emulator, the tests work well though so I moved it to the socs.testing sub-package. Let me know if this is still ok with you.

BrianJKoopman

I spent quite a bit of time today trying to track this down and still don't really know what's going on.

I can't reproduce the failing test locally -- I do, however, produce other failed tests that seem to stem from some race condition with the task completing and when the response is checked.

An example of two back to back local runs:

=================================================================== short test summary info ===================================================================
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_set_direction - assert 2 == 5
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_set_pid - assert 2 == 5
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_set_scale - assert 2 == 5
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_declare_freq - assert 2 == 5
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_tune_freq - assert 6 == 5
===================================================== 5 failed, 3 passed, 1 warning in 180.37s (0:03:00) ======================================================

=================================================================== short test summary info ===================================================================
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_set_scale - assert 2 == 5
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_declare_freq - assert 2 == 5
FAILED integration/test_hwp_pid_agent_integration.py::test_hwp_rotation_tune_freq - assert 2 == 5
===================================================== 3 failed, 5 passed, 1 warning in 192.34s (0:03:12) ======================================================

While we're not seeing the error on the CI runner, it concerns me these crop up locally. I suspect it's something to do with the new structure. Are you able to reproduce? (As a related aside, task.wait() works with this new structure, yes?)

I'll note I'm not testing with the newest ocs main branch with the updated OpCodes. And these examples were tested on Python 3.8 (like the CI runner uses.)

You probably saw that I re-ran the tests in this PR, which failed. I isolated just the PMX and PID tests in a separate run, and that passed no problem.

I've only really seen the failure here when running all tests. I'm also seeing the occasional failure of a test in common/test_pmx.py.

That said, recent runs of the CI workflow on other branches (like this merge to main from the other day) have run fine, so I'm not convinced this is just a flakey test that's suddenly cropping up, but instead something related to changes here.

socs/testing/hwp_emulator.py

tests/integration/test_hwp_pid_agent_integration.py

socs/testing/hwp_emulator.py

for more information, see https://pre-commit.ci

BrianJKoopman

Glad you got the tests working! I have two small docstring requests that I missed in the initial review, and a question. Otherwise I think this is good to go.

socs/agents/hwp_pid/agent.py

tests/integration/test_hwp_pid_agent_integration.py

BrianJKoopman

Approved! Will squash and merge.

* HWP Emulation and PID restructure * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * More complete PMX emulation * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Changes based on Brian's review * fix session.data + threading in PID agent * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixes from further testing of PID agent * remode PID start mode from docs * Fix tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix mutable defaults * Adds traceback to device-emulator exception * adds time to device emulator logs * add debug print to when readline failes * change order of tests * test * change to update_responses * test update_responses * update_responses docstring * more testing race-conditions * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Adds session data info to docstring --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

jlashner requested review from BrianJKoopman, mhasself and ykyohei January 4, 2024 20:16

jlashner mentioned this pull request Jan 4, 2024

TimeoutLock less HWP PMX agent #604

Merged

6 tasks

ykyohei reviewed Jan 5, 2024

View reviewed changes

BrianJKoopman requested changes Jan 10, 2024

View reviewed changes

ykyohei mentioned this pull request Jan 11, 2024

Changed acquisition function to release the TimeoutLock when not actively querying data #614

Closed

6 tasks

mhasself reviewed Jan 11, 2024

View reviewed changes

jlashner force-pushed the hwp-emulation branch from 0d47641 to 6f05671 Compare January 16, 2024 20:05

jlashner requested a review from BrianJKoopman January 17, 2024 16:21

BrianJKoopman reviewed Jan 22, 2024

View reviewed changes

jlashner and others added 9 commits January 23, 2024 10:54

HWP Emulation and PID restructure

c13caf4

[pre-commit.ci] auto fixes from pre-commit.com hooks

0042027

for more information, see https://pre-commit.ci

More complete PMX emulation

f96dd7b

[pre-commit.ci] auto fixes from pre-commit.com hooks

7620fea

for more information, see https://pre-commit.ci

Changes based on Brian's review

b271c89

fix session.data + threading in PID agent

26ec6c6

[pre-commit.ci] auto fixes from pre-commit.com hooks

12a907a

for more information, see https://pre-commit.ci

Fixes from further testing of PID agent

2cccf4f

remode PID start mode from docs

4bd1a64

jlashner force-pushed the hwp-emulation branch from 20a8582 to 4bd1a64 Compare January 23, 2024 15:54

jlashner and others added 2 commits January 29, 2024 17:19

Fix tests

c2b99ae

[pre-commit.ci] auto fixes from pre-commit.com hooks

b9036de

for more information, see https://pre-commit.ci

BrianJKoopman reviewed Feb 1, 2024

View reviewed changes

socs/testing/hwp_emulator.py Outdated Show resolved Hide resolved

tests/integration/test_hwp_pid_agent_integration.py Show resolved Hide resolved

socs/testing/hwp_emulator.py Show resolved Hide resolved

Fix mutable defaults

45d0f80

jlashner mentioned this pull request Feb 2, 2024

Failing tests on HWP Emulation Branch #629

Closed

jlashner added 2 commits February 2, 2024 11:58

Adds traceback to device-emulator exception

66e5a3a

adds time to device emulator logs

732ffc6

jlashner added 3 commits February 2, 2024 13:03

add debug print to when readline failes

026738d

change order of tests

b9e6d20

test

bfcc549

jlashner force-pushed the hwp-emulation branch from ed21a81 to bfcc549 Compare February 2, 2024 19:27

change to update_responses

45aa0e9

jlashner force-pushed the hwp-emulation branch from 60cf3a0 to 45aa0e9 Compare February 2, 2024 20:04

test update_responses

c9b816c

jlashner requested a review from BrianJKoopman February 2, 2024 21:09

jlashner and others added 3 commits February 5, 2024 10:03

update_responses docstring

964a7ec

more testing race-conditions

c88c700

[pre-commit.ci] auto fixes from pre-commit.com hooks

b40a749

for more information, see https://pre-commit.ci

BrianJKoopman reviewed Feb 5, 2024

View reviewed changes

socs/agents/hwp_pid/agent.py Show resolved Hide resolved

socs/agents/hwp_pid/agent.py Show resolved Hide resolved

socs/agents/hwp_pid/agent.py Show resolved Hide resolved

tests/integration/test_hwp_pid_agent_integration.py Show resolved Hide resolved

BrianJKoopman mentioned this pull request Feb 5, 2024

Supervisor actions #628

Merged

6 tasks

Adds session data info to docstring

41cfb30

jlashner requested a review from BrianJKoopman February 6, 2024 20:21

BrianJKoopman approved these changes Feb 6, 2024

View reviewed changes

BrianJKoopman merged commit 54289e9 into main Feb 6, 2024
7 checks passed

BrianJKoopman deleted the hwp-emulation branch February 6, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HWP Emulation and PID agent lockless restructure #606

HWP Emulation and PID agent lockless restructure #606

jlashner commented Jan 4, 2024

ykyohei left a comment

BrianJKoopman left a comment

BrianJKoopman Jan 10, 2024

mhasself Jan 11, 2024

jlashner Jan 11, 2024

jlashner commented Jan 17, 2024

BrianJKoopman left a comment

jlashner commented Jan 29, 2024

BrianJKoopman left a comment

BrianJKoopman left a comment

BrianJKoopman left a comment

		if args.mode is not None:
		agent.log.warn("--mode agrument is deprecated.")

HWP Emulation and PID agent lockless restructure #606

HWP Emulation and PID agent lockless restructure #606

Conversation

jlashner commented Jan 4, 2024

Description

How Has This Been Tested?

Types of changes

Checklist:

ykyohei left a comment

Choose a reason for hiding this comment

BrianJKoopman left a comment

Choose a reason for hiding this comment

BrianJKoopman Jan 10, 2024

Choose a reason for hiding this comment

mhasself Jan 11, 2024

Choose a reason for hiding this comment

jlashner Jan 11, 2024

Choose a reason for hiding this comment

jlashner commented Jan 17, 2024

BrianJKoopman left a comment

Choose a reason for hiding this comment

jlashner commented Jan 29, 2024

BrianJKoopman left a comment

Choose a reason for hiding this comment

BrianJKoopman left a comment

Choose a reason for hiding this comment

BrianJKoopman left a comment

Choose a reason for hiding this comment