Fix _whichmodule with multiprocessing

[hendrikmakait]

Closes #528

I don't have experience with cloudpickles test suite. Any advice on what test to take as a guideline to test this?

[jrbourbeau]

Thanks @hendrikmakait -- this looks like a reasonable change to me. For reference, here's where a similar change was made in cpython python/cpython#23403

Here's where we have an existing test for _whichmodule:

cloudpickle/tests/cloudpickle_test.py

Lines 1430 to 1480 in d003266

	def test_non_module_object_passing_whichmodule_test(self):
	# https://github.com/cloudpipe/cloudpickle/pull/326: cloudpickle should
	# not try to instrospect non-modules object when trying to discover the
	# module of a function/class. This happenened because codecov injects
	# tuples (and not modules) into sys.modules, but type-checks were not
	# carried out on the entries of sys.modules, causing cloupdickle to
	# then error in unexpected ways
	def func(x):
	return x**2
	# Trigger a loop during the execution of whichmodule(func) by
	# explicitly setting the function's module to None
	func.__module__ = None
	class NonModuleObject:
	def __ini__(self):
	self.some_attr = None
	def __getattr__(self, name):
	# We whitelist func so that a _whichmodule(func, None) call
	# returns the NonModuleObject instance if a type check on the
	# entries of sys.modules is not carried out, but manipulating
	# this instance thinking it really is a module later on in the
	# pickling process of func errors out
	if name == "func":
	return func
	else:
	raise AttributeError
	non_module_object = NonModuleObject()
	assert func(2) == 4
	assert func is non_module_object.func
	# Any manipulation of non_module_object relying on attribute access
	# will raise an Exception
	with pytest.raises(AttributeError):
	_ = non_module_object.some_attr
	try:
	sys.modules["NonModuleObject"] = non_module_object
	func_module_name = _whichmodule(func, None)
	assert func_module_name != "NonModuleObject"
	assert func_module_name is None
	depickled_func = pickle_depickle(func, protocol=self.protocol)
	assert depickled_func(2) == 4
	finally:
	sys.modules.pop("NonModuleObject")

For this case, I think it'd be sufficient to add a test that does something along the lines of:

assert cloudpickle.cloudpickle._whichmodule(foo, foo.__name__) == <expected-result>

Also, it looks like CI didn't run for this PR. GitHub had some downtime earlier today, so maybe that's related. You might consider pushing an empty commit to see if that helps.

[hendrikmakait]

Thanks, @jrbourbeau! I've added a test now. It seems to require the optional dependency on numpy, at least I haven't been able to reproduce the original issue with the standard library.

Regarding the workflows, it looks like they require maintainer approval:

This workflow requires approval from a maintainer. Learn more about approving workflows.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.12%. Comparing base (d003266) to head (bde4e14).

❗ Current head bde4e14 differs from pull request most recent head a8c729f. Consider uploading reports for the commit a8c729f to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #529   +/-   ##
=======================================
  Coverage   96.12%   96.12%           
=======================================
  Files           3        3           
  Lines         568      568           
  Branches      123      123           
=======================================
  Hits          546      546           
  Misses         11       11           
  Partials       11       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hendrikmakait]

@jrbourbeau: Let me know if there's anything else left to do here.

[ogrisel]

I confirm that the non-regression test fails with the previous version of the code and that the fix works.

LGTM, thanks for the fix (I just pushed a cosmetic change and an entry for the changelog).