-
Notifications
You must be signed in to change notification settings - Fork 227
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
Improved rclpy import failed message #12
Conversation
50c8d03
to
ae54ea7
Compare
I also addressed the style problems from #10 (comment). |
b2f62dd
to
ae54ea7
Compare
Actually, scratch that. I remove the commit that cleans up #10 so I can put it in a different pr. |
this excepthook allows you to register something to be printed for an exception, but only if it goes unhandled.
…tensions available
ca04d1f
to
d81e9fd
Compare
try: | ||
__rmw_implementation_module = importlib.import_module(module_name, package='rclpy') | ||
except ImportError as exc: | ||
if "No module named 'rclpy._rclpy__" in str(exc): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you could do slightly better than hardcoding the module prefix here. If the module name changed from rclpy._rclpy__rmw_....
to something else then this logic would break. Wouldn't something like "No module named 'rclpy.{0}".format(module_name)
work?
I fixed http://ci.ros2.org/job/ci_linux/1174/testReport/junit/projectroot.home.rosbuild.ci_scripts.ws.src.ros2.rclpy/rclpy/pep257/ in b994e52, it shouldn't need a retest. |
I believe all the CI is satisfactory (with my pep257 fix), looking for final reviews before squash and merge. |
bump |
+1 |
if available_implementations: | ||
select_rmw_implementation(available_implementations[0]) # select the first one | ||
else: | ||
raise NoImplementationAvailableException() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the opposite order:
if not available_implementations:
raise NoImplementationAvailableException()
select_rmw_implementation(available_implementations[0]) # select the first one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes a difference, but if you want to change it I'm fine with that.
I achieved this by adding a helpful message which prints when the import fails, but only if it goes unhandled.
I wanted to avoid printing it always incase someone is explicitly trying each of the implementations and wants to avoid the logging output. To accomplish this I added a custom
excepthook
forrclpy
to override the built-insys.excepthook
. You can also add "addendum's" for uncaught exceptions, which will printed before the traceback. In the customexcepthook
, it looks for a matching addendum, prints it if it finds one, and then passes along the unhandled exception to the originalexcepthook
.This is the observed behavior: