ord: register Tech for Python global APIs#10919
Conversation
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
There was a problem hiding this comment.
Code Review
This pull request introduces a new regression test tech_lifecycle to verify the lifecycle and thread count configuration of the Tech class in Python. To support this, src/Tech.cc has been updated to properly set and reset the global OpenRoad instance during the construction and destruction of Tech. Additionally, the aes_nangate45 test in src/drt/test/BUILD has been simplified by migrating from regression_rule_test to regression_test. There are no review comments, so I have no feedback to provide.
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
|
Black caught an extra blank line in the new lifecycle regression. I fixed it and reran the lifecycle test, the full AES Python regression, and the lint build on the VM. All pass. The earlier Mac failure was a timeout downloading Boost, so the new push will retry that job as well. |
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
|
Jenkins exposed a standalone startup bug in the previous update. I reproduced the Jenkins failure on the VM before the fix. After the fix, the Python lifecycle test, |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 963f273fd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| set_thread_count(2) | ||
| assert thread_count() == 2 |
There was a problem hiding this comment.
Avoid assuming two hardware threads
On runners that expose only one CPU, set_thread_count(2) is clamped by OpenRoad::setThreadCount to std::thread::hardware_concurrency(), so thread_count() returns 1 and this lifecycle regression fails even though the registration works. Use a value guaranteed to be valid, such as 1, or assert against the clamped value.
Useful? React with 👍 / 👎.
Direct Python users can construct a
Techobject and then call module-level APIs such asset_thread_count(). Those APIs use the process-wide OpenROAD instance, butTechdid not register the instance it owns. The result was a null dereference.The DRT AES Python target also did not carry the OpenROAD Python runfiles, so it failed at import time and never reached this crash.
This change registers the
Techinstance during construction and clears it during destruction when it is still active. It adds a small lifecycle regression and moves the AES target through the standard Python regression macro.VM testing:
The normal AES run completed in 132 seconds. The ASan run completed in 946 seconds with leak detection disabled.