Factories: do not explicitly check type of entry point if `load=False` #5352

sphuber · 2022-02-16T11:23:35Z

The recent release of setuptools==60.9.0 caused the CI to start
failing. The CalculationFactory started raising, when invoked through
the PluginParamType, when load=False because the loaded entry points
were no longer an instance of importlib_metadata.EntryPoint. The
reason for this change is that as of that setuptools release, it
started vendoring the importlib_metadata package and so it overrides
the type to setuptools._vendor.importlib_metadata.EntryPoint for the
loaded entry points.

For reasons unknown, this behavior of the changing of the type of loaded
entry points, only manifested when invoking the CLI through pytest.
When calling the failing CLI command directly, it would work just fine.

One solution to this problem would have been to extend the check to also
allow the vendored entry point type of setuptools but this seems
fragile. Really, it seems that the original design of checking the type
of the entry point in the case of load=False is unnecessary. There is
no reason to suspect that get_entry_point should return anything that
does not behave as an entry point if it didn't except. It is better to
be Pythonic here and rely on duck typing. If what is returned by the
factory behaves like an EntryPoint instance, regardless of its exact
module and class, then that is all that matters.

chrisjsewell · 2022-02-16T11:34:46Z

Cheers @sphuber, If you look at d4e80e2 (#5330), you will see that there are also some tests that check the isinstance.

The problem with checking this fix, is that it only manifests in the test-install workflows, as opposed to the ci-code workflows that are running here.
the .github/workflows/test-install.yml workflows are only "active" if you change one of the files that changes requirements

sphuber · 2022-02-16T11:43:46Z

I see, the difference between those two workflows is that ci-code.yml does not explicitly update setuptools, whereas test-install.yml does. I changed this in d3c9ac6 . I think this should really be considered a bug in setuptools should it not? Don't think we can restrict that version though. With the solution of this PR, the bug should no longer be present in the code. It is just the tests/plugins/test_plugin/test_entry_points.py that needs to be fixed. I added that test to check that the deprecation warnings of old entry point names without the core. prefix works correctly. There is no need to be checking the instance of the return value. So I would simply be tempted to remove it.

chrisjsewell · 2022-02-16T11:44:39Z

If what is returned by the factory behaves like an EntryPoint instance, regardless of its exact module and class, then that is all that matters.

This could be problematic:

=================================== FAILURES ===================================
________________ test_code_duplicate_non_interactive[vim -cwq] _________________

run_cli_command = <function run_cli_command.<locals>._run_cli_command at 0x7f07fe6cb310>
code = <Code: Remote code 'code' on localhost-test, pk: 1440, uuid: b0b82702-e3cd-42b2-a6f1-5ed3d4aaf35e>
non_interactive_editor = None

    @pytest.mark.usefixtures('aiida_profile_clean')
    @pytest.mark.parametrize('non_interactive_editor', ('vim -cwq',), indirect=True)
    def test_code_duplicate_non_interactive(run_cli_command, code, non_interactive_editor):
        """Test code duplication non-interactive."""
        label = 'code_duplicate_noninteractive'
        run_cli_command(cmd_code.code_duplicate, ['--non-interactive', f'--label={label}', str(code.pk)])
    
        duplicate = load_code(label)
        assert code.description == duplicate.description
        assert code.get_prepend_text() == duplicate.get_prepend_text()
        assert code.get_append_text() == duplicate.get_append_text()
>       assert code.get_input_plugin_name() == duplicate.get_input_plugin_name()
E       assert 'core.arithmetic.add' == "EntryPoint(n...alculations')"
E         - EntryPoint(name='core.arithmetic.add', value='aiida.calculations.arithmetic.add:ArithmeticAddCalculation', group='aiida.calculations')
E         + core.arithmetic.add

It seems like perhaps str(EntryPoint(...)) (used in set_input_plugin_name) is different for the two variants 🤷

sphuber · 2022-02-16T11:49:25Z

I think Code.set-input_plugin_name should expect a string though with the entry point name, e.g. 'core.arithmetic.add', and nothing else. If client code is currently passing an EntryPoint somewhere, then we should change that and have it pass EntryPoint.name instead. set_plugin_name should then do type_check(input_plugin_name, str) and remove the str() cast.

codecov · 2022-02-16T11:49:43Z

Codecov Report

Merging #5352 (1f99246) into develop (1aed026) will decrease coverage by 0.01%.
The diff coverage is 80.65%.

@@             Coverage Diff             @@
##           develop    #5352      +/-   ##
===========================================
- Coverage    82.14%   82.13%   -0.00%     
===========================================
  Files          533      533              
  Lines        38491    38510      +19     
===========================================
+ Hits         31613    31625      +12     
- Misses        6878     6885       +7

Flag	Coverage Δ
django	`77.21% <80.65%> (-<0.01%)`	⬇️
sqlalchemy	`76.51% <80.65%> (-<0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/orm/utils/builders/code.py	`84.49% <ø> (-0.39%)`	⬇️
aiida/plugins/factories.py	`88.80% <79.32%> (-3.58%)`	⬇️
aiida/cmdline/commands/cmd_code.py	`90.62% <100.00%> (+0.08%)`	⬆️
aiida/engine/daemon/client.py	`75.38% <0.00%> (-1.00%)`	⬇️
aiida/transports/plugins/local.py	`81.71% <0.00%> (+0.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1aed026...1f99246. Read the comment docs.

chrisjsewell · 2022-02-16T11:56:53Z

I think this should really be considered a bug in setuptools should it not?

Its certainly not ideal 😒, but then again I think we would not have to have setuptools installed, if it was not for the pymatgen issue, since build-system dependencies are usually installed in isolated environments

sphuber · 2022-02-16T12:01:18Z

Its certainly not ideal, but then again I think we would not have to have setuptools installed, if it was not for the pymatgen issue, since build-system dependencies are usually installed in isolated environments

That's the thing though. I removed the setuptools update from the ci-code.yml because it was no longer necessary. There is a separate step to install pymatgen in a compatible way though. However, since they now use the oldest-compatible-numpy metapackage, recent versions should be installable without the workaround.

But even if we could technically install without setuptools, there is no guaranteeing that people won't have it in their environment through some other means. So we cannot rely on it never being present and so we have to deal with this bug anyway. 😞

sphuber · 2022-02-16T12:18:42Z

It seems like perhaps str(EntryPoint(...)) (used in set_input_plugin_name) is different for the two variants shrug

I actually cannot reproduce this:

aiida_py38) sph@citadel:~/code/aiida/env/release/aiida-core$ ipython
Python 3.8.10 (default, Nov 26 2021, 20:14:08) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.31.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import setuptools

In [2]: setuptools.__version__
Out[2]: '60.9.1'

In [3]: from importlib_metadata import entry_points

In [4]: ep = entry_points().select(group='aiida.calculations', name='core.arithmetic.add')['core.arithmetic.add']

In [5]: type(ep)
Out[5]: setuptools._vendor.importlib_metadata.EntryPoint

In [6]: str(ep)
Out[6]: "EntryPoint(name='core.arithmetic.add', value='aiida.calculations.arithmetic.add:ArithmeticAddCalculation', group='aiida.calculations')"

versus

(aiida_py38) sph@citadel:~/code/aiida/env/release/aiida-core$ ipython
Python 3.8.10 (default, Nov 26 2021, 20:14:08) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.31.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import setuptools

In [2]: setuptools.__version__
Out[2]: '60.8.0'

In [3]: from importlib_metadata import entry_points

In [4]: ep = entry_points().select(group='aiida.calculations', name='core.arithmetic.add')['core.arithmetic.add']

In [5]: type(ep)
Out[5]: importlib_metadata.EntryPoint

In [6]: str(ep)
Out[6]: "EntryPoint(name='core.arithmetic.add', value='aiida.calculations.arithmetic.add:ArithmeticAddCalculation', group='aiida.calculations')"

Seems like the __str__ implementation is the same for both?

chrisjsewell · 2022-02-16T12:22:59Z

I actually cannot reproduce this

I definitely feel though, that you should make a minor change in e.g. pyproject.toml, in this PR, so that the test-install workflows are activated, and we can be totally sure that they are all passing

chrisjsewell · 2022-02-16T12:24:47Z

Well anyhow, we are now seeing the same test failure in ci-code: https://github.com/aiidateam/aiida-core/runs/5215699280?check_suite_focus=true

sphuber · 2022-02-16T12:35:04Z

I definitely feel though, that you should make a minor change in e.g. pyproject.toml, in this PR, so that the test-install workflows are activated, and we can be totally sure that they are all passing

I changed ci-code.yml instead to reinstate the installation of recent setuptools to make it do the same as test-install.yml. This is why the tests are now failing. I am now adding a fix for the test you mentioned. But what I am changing instead is that the CodeBuilder is not passing an EntryPoint to set_input_plugin_name but the string name. That should fix the problem.

I am tempted to also change Code.set_input_plugin_name as I described, by adding a type check to only accept str and remove the cast to str, but that could maybe break some plugins. I think it is unlikely, because I don't think people load EntryPoints to create new Code instances.

chrisjsewell · 2022-02-16T12:39:40Z

to make it do the same as test-install.yml.

Well it does not do exactly the same as that, since one installs the hard-pinned requirements from the lock files, and one installs the looser pinnings from the install_requires.
But, as long as the same test failures are now manifesting and getting fixed, then its not as much of an issue 👍

The recent release of `setuptools==60.9.0` caused the CI to start failing. The `CalculationFactory` started raising, when invoked through the `PluginParamType`, when `load=False` because the loaded entry points were no longer an instance of `importlib_metadata.EntryPoint`. The reason for this change is that as of that `setuptools` release, it started vendoring the `importlib_metadata` package and so it overrides the type to `setuptools._vendor.importlib_metadata.EntryPoint` for the loaded entry points. For reasons unknown, this behavior of the changing of the type of loaded entry points, only manifested when invoking the CLI through `pytest`. When calling the failing CLI command directly, it would work just fine. One solution to this problem would have been to extend the check to also allow the vendored entry point type of `setuptools` but this seems fragile. Really, it seems that the original design of checking the type of the entry point in the case of `load=False` is unnecessary. There is no reason to suspect that `get_entry_point` should return anything that does not behave as an entry point if it didn't except. It is better to be Pythonic here and rely on duck typing. If what is returned by the factory behaves like an `EntryPoint` instance, regardless of its exact module and class, then that is all that matters. The test `tests/plugins/test_entry_point.py` was also explicitly checking the type of the return value of `get_entry_point` but is shouldn't as its purpose is just to verify the deprecation warning is emitted for deprecated entry point names, and so the check is removed.

The `CodeBuilder` could receive the `input_plugin_name` keyword argument as an `EntryPoint` from the `verdi code setup/duplicate` commands, so it included a switch to get the entry point name from it. Due to a bug in setuptools, the type of the `EntryPoint` can change under certain circumstances and so the `isinstance` check on the entry point does not hit. This causes the `EntryPoint` to be passed to the `Code.set_input_plugin_name` method, which does not check the type and casts the argument to `str`. However, the `__str__` of `EntryPoint` does not return just the name, but a more verbose representation. This will cause the `Code` to be unusable. The conversion of the `EntryPoint` to its name in the `CodeBuilder is removed and instead the CLI commands are responsible for passing the string entry point name. Ideally, `Code.set_input_plugin_name` is updated to only accept `str` arguments and remove the cast. But this would be a breaking change and so is not done for now.

chrisjsewell

Looks good to me 👍

chrisjsewell · 2022-02-16T13:29:54Z

Just gonna copy this comment here, from #5330, for prosperity before I delete it there:

So setuptools is overriding the EntryPoint class that gets returned face_with_head_bandage
this is since: pypa/setuptools#3086

Cheers for hunting this down @chrisjsewell . It seems to make sense, but I still cannot fully reproduce it, and it seems your fix also didn't do it as the tests are still hanging at that verdi code list test.

I created a Python 3.8 environment and installed a version of setuptools before (60.8.0) the vendoring of importlib_metadata and after it (60.9.1). I can see the class of the entry point being changed, however, the factory still works fine, so at least on my machine the isinstance(entry_point, EntryPoint) doesn't seem to notice the difference.

This is behavior with old version

(aiida_py38) sph@citadel:~/code/aiida/env/release/aiida-core$ ipython
Python 3.8.10 (default, Nov 26 2021, 20:14:08) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.31.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import setuptools

In [2]: setuptools.__version__
Out[2]: '60.8.0'

In [3]: from aiida.plugins.entry_point import get_entry_point
   ...: ep = get_entry_point('aiida.calculations', 'core.arithmetic.add')
   ...: print(type(ep), ep.__module__, ep.__class__)
<class 'importlib_metadata.EntryPoint'> importlib_metadata <class 'importlib_metadata.EntryPoint'>

In [4]: from aiida.plugins import CalculationFactory

In [5]: CalculationFactory('core.arithmetic.add')
Out[5]: aiida.calculations.arithmetic.add.ArithmeticAddCalculation

We get the class that we expect.

And this is with the latest release

(aiida_py38) sph@citadel:~/code/aiida/env/release/aiida-core$ ipython
Python 3.8.10 (default, Nov 26 2021, 20:14:08) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.31.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import setuptools

In [2]: setuptools.__version__
Out[2]: '60.9.1'

In [3]: from aiida.plugins.entry_point import get_entry_point
   ...: ep = get_entry_point('aiida.calculations', 'core.arithmetic.add')
   ...: print(type(ep), ep.__module__, ep.__class__)
<class 'setuptools._vendor.importlib_metadata.EntryPoint'> setuptools._vendor.importlib_metadata <class 'setuptools._vendor.importlib_metadata.EntryPoint'>

In [4]: from aiida.plugins import CalculationFactory

In [5]: CalculationFactory('core.arithmetic.add')
Out[5]: aiida.calculations.arithmetic.add.ArithmeticAddCalculation

Then I tried running the tests, and there I can reproduce it locally. That is when I run pytest tests/cmdline/commands/test_code.py -k test_code_list. However, runnning the exact same command as the test does but directly as verdi code list works just fine. Now here is the whopper, when I add logging, I see that the type of the loaded entry point is different when I run through pytest compared to direct invocation 🤯

So conclusion so far: the problem only appears for:

Python < 3.10
setuptools>=60.9
When running through pytest (normal invocation of CLI works just fine)

Honestly no idea what could cause the difference with pytest.

I also note that the bug appears when running just in ipython:

(aiida_py38) sph@citadel:~/code/aiida/env/release/aiida-core$ ipython
Python 3.8.10 (default, Nov 26 2021, 20:14:08) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.31.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import setuptools

In [2]: setuptools.__version__
Out[2]: '60.9.1'

In [3]: from aiida.plugins import CalculationFactory

In [4]: ep = CalculationFactory('core.arithmetic.add', load=False)
HERE False core.arithmetic.add <class 'setuptools._vendor.importlib_metadata.EntryPoint'>
---------------------------------------------------------------------------
InvalidEntryPointTypeError                Traceback (most recent call last)
<ipython-input-4-1fbbfceb5d72> in <module>
----> 1 ep = CalculationFactory('core.arithmetic.add', load=False)

~/code/aiida/env/release/aiida-core/aiida/plugins/factories.py in CalculationFactory(entry_point_name, load)
     86         return entry_point
     87 
---> 88     raise_invalid_type_error(entry_point_name, entry_point_group, valid_classes)
     89 
     90 

~/code/aiida/env/release/aiida-core/aiida/plugins/factories.py in raise_invalid_type_error(entry_point_name, entry_point_group, valid_classes)
     42     template = 'entry point `{}` registered in group `{}` is invalid because its type is not one of: {}'
     43     args = (entry_point_name, entry_point_group, ', '.join([e.__name__ for e in valid_classes]))
---> 44     raise InvalidEntryPointTypeError(template.format(*args))
     45 
     46 

InvalidEntryPointTypeError: entry point `core.arithmetic.add` registered in group `aiida.calculations` is invalid because its type is not one of: CalcJob, calcfunction

So the real question might be why running this same code path through invoking verdi code list does not incur this exception?

(aiida_py38) sph@citadel:~/code/aiida/env/release/aiida-core$ verdi code list -A -a -o --input-plugin=core.arithmetic.add --computer=localhost
HERE False core.arithmetic.add <class 'importlib_metadata.EntryPoint'>
# List of configured codes:
# (use 'verdi code show CODEID' to see the details)
* pk 1 - bash@localhost (aiida@localhost)

sphuber mentioned this pull request Feb 16, 2022

🔀 MERGE: Remove Django storage backend #5330

Merged

sphuber force-pushed the fix/factory-entry-point-type-check branch from 74ff375 to 19f3596 Compare February 16, 2022 11:56

sphuber added 2 commits February 16, 2022 13:42

sphuber force-pushed the fix/factory-entry-point-type-check branch from f5b7579 to 1f99246 Compare February 16, 2022 12:45

chrisjsewell approved these changes Feb 16, 2022

View reviewed changes

sphuber merged commit ea0f447 into aiidateam:develop Feb 16, 2022

sphuber deleted the fix/factory-entry-point-type-check branch February 16, 2022 13:15

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factories: do not explicitly check type of entry point if `load=False` #5352

Factories: do not explicitly check type of entry point if `load=False` #5352

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022

sphuber commented Feb 16, 2022

codecov bot commented Feb 16, 2022 •

edited

Loading

chrisjsewell commented Feb 16, 2022 •

edited

Loading

sphuber commented Feb 16, 2022

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022 •

edited

Loading

chrisjsewell commented Feb 16, 2022

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022

chrisjsewell left a comment •

edited

Loading

chrisjsewell commented Feb 16, 2022

Factories: do not explicitly check type of entry point if load=False #5352

Factories: do not explicitly check type of entry point if load=False #5352

Conversation

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022

sphuber commented Feb 16, 2022

codecov bot commented Feb 16, 2022 • edited Loading

Codecov Report

chrisjsewell commented Feb 16, 2022 • edited Loading

sphuber commented Feb 16, 2022

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022 • edited Loading

chrisjsewell commented Feb 16, 2022

sphuber commented Feb 16, 2022

chrisjsewell commented Feb 16, 2022

chrisjsewell left a comment • edited Loading

Choose a reason for hiding this comment

chrisjsewell commented Feb 16, 2022

Factories: do not explicitly check type of entry point if `load=False` #5352

Factories: do not explicitly check type of entry point if `load=False` #5352

codecov bot commented Feb 16, 2022 •

edited

Loading

chrisjsewell commented Feb 16, 2022 •

edited

Loading

chrisjsewell commented Feb 16, 2022 •

edited

Loading

chrisjsewell left a comment •

edited

Loading