Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package according modern guidelines #32

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

rominf
Copy link
Contributor

@rominf rominf commented May 18, 2024

What does this PR do?

See commit messages and linked issue for more details.

  • Package according modern guidelines
  • Automatic reformatting
  • Address flake8 errors

Fixes #31

Before submitting, make sure that you can tick all these boxes.

  • Did you read the contributor guideline?
  • Is your PR against the dev branch?
    dev branch is out-of-date, hence using master.
  • Did you write any new necessary tests for testing new functionality?
    Not applicable.
  • Did you write comprehensive documentation/docstrings (if applicable)?
    Not applicable.
  • Did you successfully run the tests with pytest tests/?
  • Did you ensure a consistent style with make style and make quality

@rominf
Copy link
Contributor Author

rominf commented Jun 20, 2024

Hi @BramVanroy! Could you please have a look?

@BramVanroy
Copy link
Owner

Hi @BramVanroy! Could you please have a look?

Hi there! You're definitely right that this package could use a refactor and bump to new standards. We can/should probably also deprecate support for earlier python versions that are approaching EOL.

That said, I'm not a hatchling person (just setuptools works best for me) so I rather not merge this as it is now. If you can refactor to use plain setuptools (example: https://github.com/BramVanroy/dutch-instruction-datasets/) I can reconsider. Or I can do it over summer if you can wait that long.

Thanks

@rominf rominf force-pushed the rominf-fix-setup branch from 83466dd to 66c9af5 Compare June 20, 2024 11:12
rominf added 3 commits June 20, 2024 14:14
closes BramVanroy#31

Project repackaged accoding modern Python packaging guidelines:
https://packaging.python.org/en/latest/

The main reason for repackaging is that previously it was not possible to
build/install package in virtualenv when spacy was not installed in
system environment, because `setup.py` imported all project modules
indirectly through importing `__init__.py`:
```
$ python3 -m venv .venv
$ source .venv/bin/activate
$ pip install .
Processing /home/rominf/dev/spacy_conll
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  error: subprocess-exited-with-error

  × Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [24 lines of output]
      Traceback (most recent call last):
        File "/usr/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
          main()
        File "/usr/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 335, in main
          json_out['return_val'] = hook(**hook_input['kwargs'])
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/usr/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 118, in get_requires_for_build_wheel
          return hook(config_settings)
                 ^^^^^^^^^^^^^^^^^^^^^
        File "/tmp/pip-build-env-tbqma0n0/overlay/lib/python3.12/site-packages/setuptools/build_meta.py", line 325, in get_requires_for_build_wheel
          return self._get_build_requires(config_settings, requirements=['wheel'])
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/tmp/pip-build-env-tbqma0n0/overlay/lib/python3.12/site-packages/setuptools/build_meta.py", line 295, in _get_build_requires
          self.run_setup()
        File "/tmp/pip-build-env-tbqma0n0/overlay/lib/python3.12/site-packages/setuptools/build_meta.py", line 487, in run_setup
          super().run_setup(setup_script=setup_script)
        File "/tmp/pip-build-env-tbqma0n0/overlay/lib/python3.12/site-packages/setuptools/build_meta.py", line 311, in run_setup
          exec(code, locals())
        File "<string>", line 4, in <module>
        File "/home/rominf/dev/spacy_conll/spacy_conll/__init__.py", line 3, in <module>
          from .formatter import ConllFormatter
        File "/home/rominf/dev/spacy_conll/spacy_conll/formatter.py", line 5, in <module>
          from spacy.language import Language
      ModuleNotFoundError: No module named 'spacy'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> See above for output.
```

Replace supported Python versions with actual.

Also, switch to dynamic versioning based on git tags, see:
https://setuptools-scm.readthedocs.io/en/stable
@rominf rominf force-pushed the rominf-fix-setup branch from 66c9af5 to f822a34 Compare June 20, 2024 11:14
@rominf
Copy link
Contributor Author

rominf commented Jun 20, 2024

I replaced hatch(ling) with setuptools as you suggested and updated Python versions. Also, I moved source to src as per example. I hesitated whether to move black configuration to pyproject.toml or not. I kept it in Makefile, but I am open moving it to pyproject.toml.

@rominf
Copy link
Contributor Author

rominf commented Jul 2, 2024

Hi @BramVanroy! Is there something else I need to do here so that you reconsider?

@BramVanroy BramVanroy merged commit 97e28da into BramVanroy:master Jul 2, 2024
@BramVanroy
Copy link
Owner

Thanks a lot @rominf!

@rominf rominf deleted the rominf-fix-setup branch July 2, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It is not possible to install package in virtualenv if system environment has no spacy installed
2 participants