Skip to content

Comments

bazel: configure timeout times in .bazelrc#7085

Merged
maliberty merged 1 commit intoThe-OpenROAD-Project:masterfrom
hzeller:feature-20250409-bazelrc-timeout
Apr 9, 2025
Merged

bazel: configure timeout times in .bazelrc#7085
maliberty merged 1 commit intoThe-OpenROAD-Project:masterfrom
hzeller:feature-20250409-bazelrc-timeout

Conversation

@hzeller
Copy link
Contributor

@hzeller hzeller commented Apr 9, 2025

... so that it is not necessary to remember the flag everyt time one runs the flow tests.

... so that it is not necessary to remember the flag everyt time
one runs the flow tests.

Signed-off-by: Henner Zeller <hzeller@google.com>
@hzeller
Copy link
Contributor Author

hzeller commented Apr 9, 2025

CC: @oharboe

@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

clang-tidy review says "All clean, LGTM! 👍"

common --check_direct_dependencies=off

# Test timeouts for various levels.
test --test_timeout=300,1800,1800,9600
Copy link
Member

Choose a reason for hiding this comment

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

This does not match the removed --test_timeout=3600,3600,3600,3600

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. Let's say it is refined.

Copy link
Member

Choose a reason for hiding this comment

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

I thought the goal was the avoid timeouts without having to classify the test cases now. This seems to open the door the previous problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have essentially a bimodal set of tests. The ones that are small, and the other ones. The small ones typically run in < 1min (and mostly < 1 sec actually), so I gave them 5min.

The big flow tests are large and actually can take longer than 1h, so I gave them a good chunk.

The other test kinds are not used, so I gave them something in the middle.

@maliberty maliberty enabled auto-merge April 9, 2025 23:02
@maliberty maliberty merged commit 06ef2cb into The-OpenROAD-Project:master Apr 9, 2025
12 checks passed
@oharboe
Copy link
Collaborator

oharboe commented Apr 10, 2025

@QuantamHD Ref my writeup on thoughts on testing policy and strategy, .tcl tests are C++ unit-tests in lieu. They should take seconds, not minutes, hours and days. It is the job of ORFS and bazel-orfs(megaboom is on the order of days an occasional post-merge testing) to do integration testing that takes minutes, hours and days. I don't know what itch the hour long .tcl tests in OpenROAD scratches that ORFS can't or shouldn't, or if those test simply predate ORFS in some way.

To have good coverage in C++ unit-tests where no test takes more than seconds in OpenROAD is a pretty lofty goal and may not be expedient, so a few unit-tests (.tcl or C++) that takes minutes in OpenROAD is a pretty good state of affairs.

If that is the direction we're moving in, I don't see an issue with this pull request.

If we get lots of tests in the minutes ++ range, we'll have to revisit this. I don't see a point in preparing for that day, this is as good a way as any to put thing aside or it might be a permanent solution. It effectively keeps timeouts disabled, so it shouldn't make a difference meanwhile.

@hzeller
Copy link
Contributor Author

hzeller commented Apr 10, 2025

The long ones are the //test:flow_tests - one of them (jpeg something something) takes about 1.5h on my (probably too small) personal machine - this is why I choose a somewhat large value for the huge tests.

The small tcl tests are observed typically less than a second and already behave speed-wise like c++ unit tests; there are a few that are currently in the 70-90ish seconds; so that is what the 300s are addressing.

Anyway, I moved the numbers from the CI to the .bazelrc to make sure to not have to remember setting the timeout value when running manual tests on the development machine (and it will automatically cover the CI as well). But I also didn't want to reserve 1h for small tests, so reduced that to an easily fitting observed value, and expanded the long tests from 1h as I've observed longer times there.

Over time, the tests and categories will probably evolve.

@oharboe
Copy link
Collaborator

oharboe commented Apr 10, 2025

@hzeller Yep... //test:flow_tests are long running and marked as manual. They don't offer any testing value for bazel build as such. I think @maliberty said they are enabled in CI, but I think their value is to test the algorithms of OpenROAD in some way.

@hzeller
Copy link
Contributor Author

hzeller commented Apr 10, 2025

yep, but if I run them manually, and they time out after 1h I get reminded that I should've set the timeout ... that is why it is good to have that taken care of the .bazelrc :)

@oharboe
Copy link
Collaborator

oharboe commented Apr 10, 2025

yep, but if I run them manually, and they time out after 1h I get reminded that I should've set the timeout ... that is why it is good to have that taken care of the .bazelrc :)

I wonder if these tests will simply be retired in favor of ORFS... First I need to learn more about what they are testing to have an opinion if they should be retired.

@hzeller
Copy link
Contributor Author

hzeller commented Apr 10, 2025

It looks like they are making some medium designs, like Ibex risc-V design to kindof to end-to-end test .So having a few of these in OpenRoad like in //test:flow_tests is probably good, but other, bigger etc tests should be maintained outside as you suggest.

@oharboe
Copy link
Collaborator

oharboe commented Apr 10, 2025

It looks like they are making some medium designs, like Ibex risc-V design to kindof to end-to-end test .So having a few of these in OpenRoad like in //test:flow_tests is probably good, but other, bigger etc tests should be maintained outside as you suggest.

I'm pushing for doing bazel testing of OpenROAD from within ORFS. If these flow tests test nothing beyond ORFS(they may test scratch some itch ORFS can't, I don't know), then I would argue that they are a liability rather than a benefit. The-OpenROAD-Project/OpenROAD-flow-scripts#3055

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