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(robot-server): Remove slow re-exports from __init__.py and protocols/__init__.py #14480

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Feb 12, 2024

Overview

This rearranges some of robot-server's internal import statements to make it significantly faster to launch certain subprocesses. See #14465 (comment) for background. This goes towards RSS-451.

This is paired with a PR in oe-core: Opentrons/oe-core#134

Benchmarks

This brings the startup time of #14465's worker processes down from ~74 sec to ~31 sec. Tested on an OT-2 with this command:

python -m timeit -r1 -n1 'import robot_server.persistence._migrations._up_to_3_helper; import robot_server.persistence.legacy_pickle; robot_server.persistence.legacy_pickle._get_legacy_ot_types()'

(The call to _get_legacy_ot_types() is to make sure we account for the cost of its dynamic import. _up_to_3_helper is a thing from my work in progress for #14465 and it may not make it into the final PR, so don't be surprised if you can't import it; but it's basically just the code for that PR's worker process, extracted to a standalone file.)

Explanation

In Python, if you have a tree like this:

some_package/
    __init__.py
        # re-export .module_1.foo, module_2.bar, module_3.baz, etc.
    module_1.py
    module_2.py
    module_3.py

And then you do this:

import some_package.module_1

The Python interpreter will evaluate module_1, module_2, and module_3, because they were mentioned in some_package/__init__.py. When you factor in all the subdependencies of module_2 and module_3, this can be very very slow.

In the case of #14465, worker processes for database migration code were wasting nearly 45 seconds importing things like FastAPI routers, which are irrelevant to them.

Review requests

We will definitely regress on this performance improvement unless we deliberately guard against it.

I propose, going forward, we adopt a house rule to "very rarely" re-export things from __init__.py files. Acceptable re-exports are ones like opentrons.protocol_api.<...>, where we have a strict public interface that we want to be ergonomic for users. All other re-exports are questionable.

Then, without those re-exports, we need a way to delineate private stuff from public stuff. I propose we lean harder into prefixing the filenames with underscores. We've already done this in a few places, but it's been pretty ad-hoc so far. Underscore-prefixed filenames are a common, but not universal, Python convention. (Examples: h11, trio, requests; counterexamples: sqlalchemy, pydantic.)

Does all of that sound good?

Then, we could take this a lot further. opentrons.protocol_engine's imports, for example, are pretty heavy. You can't just import command models without also pulling in all of the execution logic, and all of the dependencies of that execution logic, including the hardware API, etc. etc.

Test plan

Risk assessment

Pretty low.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Definitely looks good, huge uplift. I totally agree with the house style concept.

@sfoster1
Copy link
Member

@sfoster1
Copy link
Member

Oops I mean THIS build (got the refs swapped): https://github.com/Opentrons/oe-core/actions/runs/7903462058

@sfoster1
Copy link
Member

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