Use name argument with Scheduler.remove_plugin calls

[douglasdavis]

• Closes #xxxx
• Tests added / passed
• Passes black distributed / flake8 distributed / isort distributed

After #5120 we added a warning about removing plugins by instance instead of by name; there are a few places distributed where the warning was getting triggered; this should remove those warnings.

[jrbourbeau]

Thanks @douglasdavis! Looking at this build there are still a few Scheduler.remove_plugin-related warnings:

distributed/diagnostics/tests/test_progress_stream.py::test_progress_stream
distributed/diagnostics/tests/test_scheduler_plugin.py::test_async_add_remove_worker
  /home/runner/work/distributed/distributed/distributed/scheduler.py:5507: FutureWarning: Removing scheduler plugins by value is deprecated and will be disabled in a future release. Please remove scheduler plugins by name instead.
    warnings.warn(

distributed/diagnostics/tests/test_progress_stream.py::test_progress_stream
  /home/runner/work/distributed/distributed/distributed/scheduler.py:5525: FutureWarning: Removing scheduler plugins by value is deprecated and will be disabled in a future release. Please remove scheduler plugins by name instead.
    warnings.warn(

[douglasdavis]

Thanks @jrbourbeau! I think I could use your opinion on how to deal with these.

1. for test_progress_stream here's the line where remove_plugin is used:
   

   distributed/distributed/diagnostics/progress_stream.py

   Line 56 in 591bca0

   "teardown": dumps_function(remove_plugin),

   
   Should we wrap this in functools.partial?

2. for test_async_add_remove_worker we're testing the exception for multiple instances of the same plugin (which should be phased out with the new name behavior where repeated names are overwritten). Should we just remove this part of the test (since it's testing an exception raising in a scenario that is no longer supported)?

[jrbourbeau]

1. It looks like remove_plugin will forward keyword arguments
   

   distributed/distributed/diagnostics/progress_stream.py

   Lines 24 to 27 in 591bca0

   	def remove_plugin(*args, **kwargs):
   	# Wrapper function around `Scheduler.remove_plugin` to avoid raising a
   	# `PicklingError` when using a cythonized scheduler
   	return Scheduler.remove_plugin(*args, **kwargs)

   
   Can we forward the correct name= keyword in this case?

2. with pytest.warns(..., match="..."): is probably what we want here. That will let us continue to test the deprecated behavior, and ensure that any appropriate warnings are emitted.

[douglasdavis]

Thanks! I think (2) is good to go. for (1) I went ahead and passed the name= via functools.partial and dropped the *args (passed to remove_plugin) in the Scheduler.remove_plugin call.

[jrbourbeau]

Hmm based on

distributed/distributed/scheduler.py

Lines 5507 to 5511 in dc47186

	warnings.warn(
	"Removing scheduler plugins by value is deprecated and will be disabled "
	"in a future release. Please remove scheduler plugins by name instead.",
	category=FutureWarning,
	)

I would have expected this to emit a FutureWarning instead of a UserWarning

[douglasdavis]

Agreed- I just made the adjustment. I wonder why it passed before

[jrbourbeau]

Interesting. This might be a bug in pytest (I opened up pytest-dev/pytest#9036). If you change the order of pytest.raises and pytest.warns, then things behave as expected

[douglasdavis]

Interesting find! Just pushed a commit to swap the order.

[jrbourbeau]

Thanks for the updates @douglasdavis! There are some tests that have begun failing which are, seemingly, unrelated to the changes here. I'm running CI against the main branch over in #5268 to further clarify.

[jrbourbeau]

Hrm, looks like these failures are unrelated...

[jrbourbeau]

Alright, merging main should fix the unrelated test failures

[jrbourbeau]

Thanks @douglasdavis! Will merge once CI finishes