Give nodes a logger

[dhood]

Minimum required to switch demos to using node loggers: logger names are not using namespaces at the moment nor calling to rcl to get their logger name.

[sloretz]

Why import this in __init__.py? Is it to initialize the root_logger before rclpy.init() is called? What breaks if this import doesn't happen here?

[dhood]

nothing breaks, it's just to save users from having to import it themselves in each script that wants to use logging. I can add a comment for it but I'm not sure which other lines in this section are doing it for the same reason

[sloretz]

I don't fully understand __init__.py, but if I comment this line out rclpy.logging appears to be imported regardless.

$ python3
Python 3.5.2 (default, Nov 17 2016, 17:05:23) 
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import rclpy
>>> rclpy.logging.logerr("Hello World!")
[ERROR] []: Hello World! (<module>() at /workspace/ros2_ws/<stdin>:1)
True

[dhood]

ah, it was necessary on master to use the non-node logging methods, but since node.py is importing it now, it's not needed here: a48d6b6

[dhood]

Please review this PR in combination with ros2/demos#190 to check that the usage is as expected. In particular, I opted for a get_logger method on the node as opposed to giving users access to the logger attribute (1) to match the other accessors, (2) in case we ever need to do something more complicated to retrieve the logger for the user.

[sloretz]

The code looks fine to me, though I didn't attend the logger design discussion.

I would rather use a logger attribute than get_logger(). Properties can be used to do something more complicated in the future..

[dhood]

@ros2/team, following @sloretz's suggestion above, what do we think about:

1. accessing the node's logger as an attribute as opposed to via an accessor
2. updating node.get_name() and node.get_namespace() to match?

We can raise when users try to set the attributes. I checked the PRs that added the other getters (#70 and #86) and there wasn't a discussion about why accessors have been used

[wjwwood]

I don't have a problem with using properties rather than getters, but I only hesitate to avoid a difference with C++ where we cannot easily do that I think.

[clalancette]

I'm for using properties, as it is more pythonic in general. But I don't really have a strong opinion either way.

[dhood]

I suppose it could be a bit counter-intuitive if it looks like an attribute but then raises when users try to set it. It might be more user-friendly to just give them the getters without setters if we don't want users to try to modify the names/logger after creation. Does that influence your opinion from a user perspective @sloretz ?

[mikaelarguedas]

May be related to the discussion (havent followed it)

rclpy/rclpy/rclpy/node.py

Lines 84 to 86 in 5c1b6e1

	@handle.setter
	def handle(self, value):
	raise AttributeError('handle cannot be modified after node creation')

[sloretz]

@dhood On point 2 my preference isn't strong enough for me to say the existing code should be changed. I think it would be fine to leave it as get_logger to stay consistent with the existing code.

[dhood]

OK it doesn't seem like we have a strong enough motivation to swap to attributes, while "intuitiveness", existing code and similarity with c++ slightly lean us towards using getters, so the conclusion I'm drawing is that we'll stick with get_logger in this PR

[dhood]

CI including changes in ros2/demos#190, will merge once comes back green

• Linux
• Linux-aarch64
• macOS
• Windows

[mikaelarguedas]

updating node.get_name() and node.get_namespace() to match?

name and namespace are not stored in Python land (not slots or attributes of the node class) so we need getters for them as we get them from rcl.

If we want to take the same approach here we would never store the logger in python and get it from rcl everytime a user want to access it (which is totally fine IMO).
If it's stored as an attribute like in this PR there is nothing preventing users from getting and setting it as any other attribute right ?

Not saying we should change it here, I'm just wondering where we expect this to be stored in the future and if the decision is made only for being consistent with rclcpp and that it'll be inconsistent within rclpy only for the time being or if we chose to treat some attributes differently than others in rclpy for the long-term.

[dhood]

name and namespace are not stored in Python land (not slots or attributes of the node class) so we need getters for them as we get them from rcl.

They wouldn't need to be stored in Python. If we swap them to properties as with

rclpy/rclpy/rclpy/node.py

Line 82 in 5c1b6e1

return self._handle

we could have the accessor call through to rcl, and then we could remove the getters.

If we want to take the same approach here we would never store the logger in python and get it from rcl everytime a user want to access it

Python has a notion of logger objects where rcl doesn't, so in this case it is appropriate for us to store the logger object associated with the node (this is what I envision for both short-term and long-term)

If it's stored as an attribute like in this PR there is nothing preventing users from getting and setting it as any other attribute right ?

That's correct, but attributes with underscores are not considered public

[dhood]

Edit (commenting so notification gets sent): "Python has a notion of logger objects where rcl doesn't" -> "rclpy has a notion of logger objects" (as of #102)