Avoid the limitation of not being able to import rclpy C extensions at module level [alternative]

[ivanpauno]

Alternative to #417.

In this case, instead of installing the C extensions as a module of rclpy package, I installed them in a new package rclpy_impl.

In this way, we don't need to "hack" the imports while testing.
This solve the same problem as #417, and also, it should solve the testing problems of rclpy on windows when launch_testing_ros is present.

[ivanpauno]

CI up to rclpy (on windows, also built up to launch_testing_ros)

• Linux (unrelated, see Fail on invalid and unknown ROS specific arguments #415 (comment))
• Linux-aarch64 (unrelated, see Fail on invalid and unknown ROS specific arguments #415 (comment))
• macOS
• Windows (unrelated, see Fail on invalid and unknown ROS specific arguments #415 (comment))

[ivanpauno]

So, this PR is solving all the test failures on Windows CI, plus allowing to import the C extensions at module level.
#417 just solves the second problem, but as proved in this CI run, it doesn't solve the first one.

[dirk-thomas]

As far as I understand this patch the C extension would immediately be loaded when import rclpy is invoked. I thought the general goal was to delay that until any function from the C extension is actually being used?

[dirk-thomas]

This package doesn't own the namespace rclpy_impl. If another package with that name installs its files they will collide with this one.

[hidmic]

I thought the general goal was to delay that until any function from the C extension is actually being used?

Hmm, why is that a goal?

[dirk-thomas]

why is that a goal?

As the comments on the local imports state: "to avoid loading extensions on module import".

First of all if you import any Python module it doesn't need to load the C extensions yet - hence it is faster and lighter weight. Second it avoid the race condition between any customization you might want to do before trying to load the C extensions. Third any tool which needs to import the Python modules but doesn't want to ever call any of the API can do so without the need to have / load the C extensions.

[hidmic]

First of all if you import any Python module it doesn't need to load the C extensions yet - hence it is faster and lighter weight.

Is there an observable difference in speed? Do we have a measure of how bigger the memory footprint is? Correct me if I'm wrong but I'd expect rmw implementations to introduce the largest overhead in dynamic library loading and we're already deferring loading for those (at least on Linux, not sure about the other platforms?).

Second it avoid the race condition between any customization you might want to do before trying to load the C extensions.

What kind of customization? Are we doing so? Could a downstream package achieve the same effect by delaying rclpy imports?

Third any tool which needs to import the Python modules but doesn't want to ever call any of the API can do so without the need to have / load the C extensions.

Which goes back to the first statement, unless we apply that same logic to every single module import. I'd like to further understand why we need to give special treatment for an otherwise regular CPython module. It certainly makes it non-standard and creates issues from time to time -- which are easy to fix, sure, but only once you've understood all the implications of how we build and install rclpy.

[ivanpauno]

@hidmic @dirk-thomas I've worked in a lazy import alternative, but I first want to finish with this discussion.

I understand @hidmic point, I think we're doing kind of a "premature optimization".
In general, _rclpy C extension will be loaded sooner or later, so I don't think is bad to load it in a global import.
The others, like _rclpy_action, should be only loaded when the module that uses it is loaded (rclpy.action in this case).
This is not currently happening, as rclpy.impl.implementation_singleton is doing all the imports in the same place, so if any C extension is loaded the others too.

---

There is currently two problems:

• The installed package folder structure is different than the package source directory structure, so pytest doesn't find the C extensions if we don't do something
  • A way to solve the problem, is to split each C extensions in its own package.
• Where to do the imports of the C extensions? Use lazy import?
  • I would move each import to the module where is used and I would avoid doing lazy import (I don't see much performance benefit).

[dirk-thomas]

I think we're doing kind of a "premature optimization".

The primary goal for doing lazy load is not performance improvement. It is to avoid the current race and in use cases which don't need the C extension not requiring it to be loaded.

In general, _rclpy C extension will be loaded sooner or later, so I don't think is bad to load it in a global import.

I think you are missing some use cases:

• An application might just be interested in some constants or helper functions defined in rclpy, e.g.

  rclpy/rclpy/rclpy/node.py

  Line 78 in 158fdef

  HIDDEN_NODE_PREFIX = '_'

  In that case it would never trigger API which needs the C extensions.
• A linter doing static code analysis needs to import all modules but doesn't execute any API.

The others, like _rclpy_action, should be only loaded when the module that uses it is loaded (rclpy.action in this case).
This is not currently happening, as rclpy.impl.implementation_singleton is doing all the imports in the same place, so if any C extension is loaded the others too.

With my proposal to convert the globally initialized handles into functions providing a lazily loaded singleton instance of the C extension that would not be the case anymore and each library would be loaded only when needed and on demand.

The installed package folder structure is different than the package source directory structure, so pytest doesn't find the C extensions if we don't do something

• A way to solve the problem, is to split each C extensions in its own package.

See my above comment why I don't think that is feasible. Also the current approach would work just fine as is. In the cases it currently fails due to the race it would work once we do lazy loading.

[ivanpauno]

Closing, because this is outdated.
We should still investigate ways of solving this issue.

[dirk-thomas]

We should still investigate ways of solving this issue.

@ivanpauno Is there any open ticket for this? If not, please create one and reference the previous approaches.

Also is the branch still needed?

[ivanpauno]

@ivanpauno Is there any open ticket for this? If not, please create one and reference the previous approaches.

See #560.

Also is the branch still needed?

Deleted.