Add params and type hints to stop() #6075

[shalearkane]

Fixes issue #6075

Overview

rucio/lib/rucio/daemons/conveyor/preparer.py

Lines 42 to 47 in f9d52b7

	def stop():
	"""
	Graceful exit.
	"""
	graceful_stop.set()

should accept parameters signalnum and handler before being used as as signal handler in

rucio/bin/rucio-conveyor-preparer

Lines 23 to 28 in f9d52b7

	def main(args):
	signal.signal(signal.SIGTERM, stop)
	try:
	run(once=args.run_once, threads=args.threads, sleep_time=args.sleep_time, bulk=args.bulk)
	except KeyboardInterrupt:
	stop()

according to the python docs. However the same function is being called without parameters in Line 28

rucio/bin/rucio-conveyor-preparer

Line 28 in f9d52b7

stop()

So I added *args to the function parameters to resolve the issue.

Alternative Solution

The other possible solution is to add the parameters:

def stop(signalnum, handler):

However, then for the function call in Line 28,

rucio/bin/rucio-conveyor-preparer

Line 28 in f9d52b7

stop()

I will have to pass those arguments to stop().

[dchristidis]

I think your choosing to use variadic arguments is reasonable.

This change should be applied to all daemons and not just the Conveyor Preparer. In addition, I think it’s a good opportunity to add some type hints, given that they are trivial.

[shalearkane]

On looking into other stop functions, I saw that they had parameters signum and frame which defaulted to None. So I changed the variadic argument to the same and added type hints.

[shalearkane]

I have changed the arguments of the stop functions of other daemons to *args. I added type-hint None to the return type. Should I add type hints to other functions as well?

[dchristidis]

I was surprised to see that so many stop() functions have some parameters already. I’m now not as confident about the best approach and would welcome someone else to comment. They way I see it, there are two alternatives:

1. def stop(*args: Any) -> None: It’s simple and, at least in my eyes, somewhat suggests that the arguments are ignored.
2. def stop(signum: Optional[int] = None, frame: Optional[FrameType] = None) -> None: More complex, but communicates well that this function is a signal handler.

[bari12]

I prefer the second;

[dchristidis]

Be sure to import Optional and FrameType. Frequently we do it like so:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from types import FrameType
    from typing import Optional

And then place the type hints inside strings.

Please note that the commits should be squashed together. Finally, the commit message has some room for improvement.

[shalearkane]

I have imported the Types and I have fixed imports in several files and squashed the commit. Please review the commit message. Should I make it more descriptive?

[dchristidis]

And then place the type hints inside strings.

You missed this part. Type hints are evaluated and will lead to NameError exceptions if the necessary classes are not imported at runtime.

As for the commit message:

1. It should follow the format <component>: <change_message> #<issue number>. You did well, but since you are touching multiple components, you should omit the first part (as opposed to prefixing the message with ‘bug’).
2. The rest of the message can be misleading. ‘add default arguments’ suggests that the parameters already exist, which is not the case. As such, ‘add parameters’ is more appropriate.
3. The body of the message is empty. You should use it to document why you are making this change. Explain how this is the proper signature for signal handlers and how deviating from it can lead to a TypeError.

[shalearkane]

I have updated my patch against the latest master branch and amended my commit message accordingly. Please review my changes 😄

[dchristidis]

Please review the failed tests. It would appear that there are some missing imports.

For example:
lib/rucio/daemons/conveyor/preparer.py:42:49: F821 undefined name 'FrameType'

Similarly, in some places you moved the import statement under a conditional block (i.e. if TYPE_CHECKING) for things that are already in use.

Finally, you’ll need to rebase your branch on the latest master.

[dchristidis]

This is likely to be my last request for changes. Please adjust some of the imports as noted in the comments below.

In addition, I would like to ask you to adjust the commit message. Please ensure that there is a blank line between the subject and the rest of the message, then move ‘Fixes #6075’ to the end of the subject.

[shalearkane]

Sir, could you please review the changes? I have made all the requested changes and have rebased the code against master branch.