aiidateam / aiida-testing

A pytest plugin to simplify testing of AiiDA plugins.
MIT License
5 stars 6 forks source link

added a missing __init__.py to utils dir #36

Closed DanielMarchand closed 3 years ago

DanielMarchand commented 3 years ago

Adds a __init__.py file, without which the setup does not run.

greschd commented 3 years ago

Hmm, I'm not sure this is the right thing to do here. What's the error message when __init__.py isn't there? Is it just because of the package finding mechanism?

We should be careful not to install a util module, that's far too generic a name.

For the fastentrypoints, we can un-vendor that by using a pyproject.toml like so: https://github.com/Z2PackDev/TBmodels/blob/190cac41a13c77d96b3cda498471701d7145d5ff/pyproject.toml#L2

DanielMarchand commented 3 years ago

I think maybe we can remove it altogether? In setup.py there is an otherwise broken import:

from utils import fastentrypoints # NOQA

Oddly it doesn't seem to be called anywhere, I can even comment out the line and everything runs fine. Maybe this is a leftover of something that isn't used anymore?

DanielMarchand commented 3 years ago

Just for completeness I show the input and full error output

$pip install -e .

    ERROR: Command errored out with exit status 1:
     command: /home/dmarchand/virtualenv/aiida/bin/python -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/home/dmarchand/Src/aiida-testing/setup.py'"'"'; __file__='"'"'/home/dmarchand/Src/aiida-testing/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base /tmp/pip-pip-egg-info-bivy6lmq
         cwd: /home/dmarchand/Src/aiida-testing/
    Complete output (5 lines):
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/home/dmarchand/Src/aiida-testing/setup.py", line 10, in <module>
        from utils import fastentrypoints  # NOQA
    ImportError: cannot import name 'fastentrypoints' from 'utils' (/home/dmarchand/Install/aiida-core/utils/__init__.py)
    ----------------------------------------
ERROR: Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.
greschd commented 3 years ago

It's not called anywhere, but it has an effect. It changes how entry points are resolved in editable installs.

The pyproject.toml defines this as a build dependency, after which a reasonably up-to-date pip / setuptools will fetch it automatically.

DanielMarchand commented 3 years ago

I'm doing a bit of digging into how to 'modernize' the setup part. It doesn't seem too bad I think we would just need to do the following:

  1. Add in pyproject.toml in our case I think we can just copy/paste the same one you used for TbModels
  2. Replace setup.json with setup.cfg I think this just a simple matter of reformatting
  3. Modernize setup.py. This seems a little bit trickier. There is some logic here that I"m not sure is a clear and direct translation to the modern methods.

I will need to look up how to combine requirements using the 'modern' methods though I'm sure its not too hard

EXTRAS_REQUIRE['dev'] = (
    EXTRAS_REQUIRE["docs"] + EXTRAS_REQUIRE["testing"] + EXTRAS_REQUIRE["pre-commit"]
)

This part I"m rather confused by and I'm not sure how to replace it.

        long_description=open(
            os.path.join(os.path.dirname(os.path.abspath(__file__)), 'README.md')
        ).read(),
        long_description_content_type="text/markdown",
greschd commented 3 years ago

1 & 2: yes

3: For the README, setup.cfg has syntax to just pass a filename.

I haven't actually found a good way to replace the combining of dev dependencies. You can either hard-code it (ugly), or we can decide to only define dev anyway - I don't see a huge drawback to installing all development dependencies together.

greschd commented 3 years ago

Just to mention: The two parts "introduce pyproject.toml" and "introduce setup.cfg" are actually independent, and we could do one without the other.

Introducing the [build-system] info to pyproject.toml makes the project PEP 517 compliant. The purpose of that PEP is to define a clear interface between the tool doing the installing (pip) and the build tool (in our case, setuptools). It defines the build requirements of the package, s.t. pip can set up the environment for setuptools to do its job.

The switch from plain setup.py to setup.cfg is then entirely internal to the way setuptools builds packages - other build tools would use different files.

For AiiDA plugins we'd still need to implement setup.cfg support to the registry (https://github.com/aiidateam/aiida-registry/issues/132), but since aiida-testing isn't technically an AiiDA plugin that shouldn't be an issue here.

greschd commented 3 years ago

I'm closing this PR because I don't think it's the right solution. To continue the discussion, I opened #37 instead.

@DanielMarchand let me know if you want to tackle this, or if I should do it.