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

perf: refactor to lazy interface loading #1932

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

Conversation

roelofvandijk
Copy link

@roelofvandijk roelofvandijk commented Mar 6, 2025

Relates to #1184

By moving the car_helpers.py imports to a pytest.fixture, we can shorten the collection time.

Refactors the car tests to use:

I tried to keep the diff as small as possible.

There are 3 big chunks of time spent during the pytest collection on a fresh run (after compilation):

  • first import of CANDefine, aka the packer_pyx compiled library (only slow the first run after compilation)
  • import of the interfaces in car_helpers.py (always slow)
  • import of pytest plugins

"Fresh" denotes a recompilation of the cython modules.

Collect only

pytest opendbc/car/tests --collect-only

State Fresh Second Run
master 0.56s 0.27s
PR 0.16s 0.13s

compare to

pytest opendbc/can/tests --collect-only

State Fresh Second Run
master 0.32s 0.02s
PR 0.29s 0.04s

Collection + execution

pytest opendbc/car/tests

State Fresh Second Run
master 6.42s 6.19s
PR 5.92s 5.76s

@github-actions github-actions bot added the car related to opendbc/car/ label Mar 6, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to opendbc! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • include a route or your device' dongle ID if relevant

@roelofvandijk roelofvandijk force-pushed the perf/faster_test_collection branch from a4b587c to e8bfae8 Compare March 6, 2025 19:26
@@ -19,53 +18,54 @@
# jerk is measured over half a second
JERK_MEAS_T = 0.5

@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this without changing the structure of this test too much?

Copy link
Author

@roelofvandijk roelofvandijk Mar 7, 2025

Choose a reason for hiding this comment

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

If this is to make the review easier, yes. I reduced the diff.

It seems nonsensical, because we are not parametrizing anymore over the test class, but directly over the test functions.

@roelofvandijk roelofvandijk force-pushed the perf/faster_test_collection branch from 7616b65 to 70d0f30 Compare March 7, 2025 09:04
@roelofvandijk roelofvandijk force-pushed the perf/faster_test_collection branch from 70d0f30 to 72b8475 Compare March 7, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
car related to opendbc/car/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants