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

Clean up python dependencies #1429

Merged
merged 3 commits into from May 2, 2024

Conversation

dchiller
Copy link
Contributor

@dchiller dchiller commented Apr 25, 2024

This PR makes a number of changes to how CantusDB handles python dependencies. It introduces poetry as our dependency management tool, removes a number of superfluous, unnecessary, or deprecated python dependencies, and groups dependencies to more clearly divide various environments (the in-container application environment, local development enviroments, etc.)

Dependencies are now divided into the following groups:

  1. Required dependencies. These are dependencies that are necessary in the django container for the Django application to run.
  2. debug dependencies. This currently includes django_debug_toolbar and is necessary in the container for debugging.
  3. test dependencies. These are dependencies used inside the container for the test suite.
  4. dev dependencies. There are dependencies that are useful in development outside the application container.

When the build argument PROJECT_ENVIRONMENT is set to DEVELOPMENT, the required, debug, and test dependencies are installed in the application container. When that argument is not set or is set to PRODUCTION or STAGING, only required dependencies are installed.

The following dependencies that were previously included in requirements.txt are not included in pyproject.toml as dependencies:

  • typed_ast -- this has been deprecated as of July 2023 (see typed_ast end of life (July 2023) python/typed_ast#179)
  • appdirs -- a package for defining platform-specific directories. Was not used in codebase and the application is only ever running on one platform (i.e. in the docker container).
  • asgiref -- a Django dependency, otherwise unused.
  • astroid -- a pylint dependency, otherwise unused.
  • attrs -- unused, unclear why included.
  • backports.zone-info -- unused, unclear why included.
  • certifi, chardet, charset-normalizer, idna, urllib3 -- requests package dependencies, otherwise unused.
  • click -- a black dependency, otherwise unused
  • isort -- an import sorter (dev dependency), we can use pylint for this
  • lazy-object-proxy, wrapt -- a pylint dependency, by way of astroid
  • lxml -- unused
  • mccabe -- a pylint dependency, otherwise unused
  • mypy-extensions, typing-extensions -- a mypy dependency, otherwise unused
  • pathspec==0.9.0
  • psycopg2 -- will likely be deprecated at some point for django's purposes (see https://docs.djangoproject.com/en/5.0/ref/databases/#postgresql-notes), upgraded to psycopg
  • python-dateutil, text-unidecode -- a Faker dependency, otherwise unused
  • pytz -- unused
  • regex -- unused
  • six-- a django-extra-views and django-autocomplete-light dependency, otherwise unused
  • sqlparse -- a django dependency, otherwise unused
  • toml -- unused

This PR will require changes/additions to the wiki around setting up the local development environment.

Closes #1428.

@ahankinson
Copy link
Member

Are you not using Black for code formatting?

@dchiller
Copy link
Contributor Author

dchiller commented Apr 26, 2024

We are (and we should use mypy, etc.) So when this PR is ready for review, I'll have them back in somewhere (thinking now in the form of a dev requirements.txt). It doesn't make sense to me to be installing development dependencies (black, etc.) inside the django container where they will never be used.

This whole quest came about because I tried to install the current cantusdb requirements.txt in a local virtual environment so that I could actually use the versions of the dev dependencies listed, and.... I can't (at the moment because of some psycopg2 stuff that is a separate thing), which seems to defeat the whole purpose.

@jacobdgm
Copy link
Contributor

jacobdgm commented Apr 26, 2024

curious what you think of the coverage library... I haven't tried removing it to see if something breaks - it's possible, but I don't think it's likely, that some other library depends on it.

My understanding is that it was added at the beginning of the project as a best practice to evaluate our test suite, and yet it hasn't been used for as long as I've been working on the project.

@dchiller
Copy link
Contributor Author

My understanding is that it was added at the beginning of the project as a best practice to evaluate our test suite, and yet it hasn't been used for as long as I've been working on the project.

Yes, that's my understanding of what it is. But since I'm not seeing it used anywhere.... (whether it would be a good idea to is another matter)

@ahankinson
Copy link
Member

Thinking now in the form of a dev requirements.txt

That's what poetry add --group dev [pkg] is for. You can choose to install all packages, or only those not in dev when you deploy. poetry install --only main or poetry install --without dev should get you there, I think.

@dchiller
Copy link
Contributor Author

That's what poetry add --group dev [pkg] is for. You can choose to install all packages, or only those not in dev when you deploy. poetry install --only main or poetry install --without dev should get you there, I think.

Right... I guess we could use this as the excuse to move to poetry....

@jacobdgm
Copy link
Contributor

jacobdgm commented Apr 26, 2024

My understanding is that it was added at the beginning of the project as a best practice to evaluate our test suite, and yet it hasn't been used for as long as I've been working on the project.

Yes, that's my understanding of what it is. But since I'm not seeing it used anywhere.... (whether it would be a good idea to is another matter)

Okay, so having looked into this, I think all we need is a bit of documentation on how to use coverage. I'll add it to https://github.com/DDMAL/CantusDB/wiki/Tests (update: done!)

@dchiller dchiller force-pushed the python-dependency-cleanup branch from d31143c to 316fa78 Compare May 1, 2024 11:45
@dchiller dchiller changed the title Remove deprecated and unnecessary python dependencies Clean up python dependencies May 1, 2024
@dchiller dchiller marked this pull request as ready for review May 1, 2024 12:22
@dchiller dchiller requested a review from ahankinson May 1, 2024 12:25
pyproject.toml Outdated
[tool.poetry.group.test]
optional = true

[tool.poetry.group.test.dependencies]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see where you're going with this, but wouldn't it be OK to just have dev? Do you routinely install the dev dependencies without the test ones? Or vice versa?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was waffling on whether to have separate debug and test groups, because I can't think of a case where you'd install one or not the other.

Technically, what's in the dev group currently doesn't ever need to be installed inside the container (code formatting and type checking can happen outside the container environment), whereas those dependencies that are in the debug and test groups do (because they extend, a la django-debug-toolbar, the django app itself).

So, it's a Docker-induced distinction. Does it really matter that we keep them separate? (ie. does anything break if we start installing black inside the container?) No, so we could definitely combine them.

Thoughts?

Copy link
Contributor

@lucasmarchd01 lucasmarchd01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning up these dependencies! I'm not a poetry expert, but it seems like this is moving us in the right direction for future development.

So, was the decision about the black and mypy dependencies to install them in the container or are we having a separate dev requirements.txt file?

@dchiller
Copy link
Contributor Author

dchiller commented May 2, 2024

Thanks for cleaning up these dependencies! I'm not a poetry expert, but it seems like this is moving us in the right direction for future development.

Thanks! I'm going to make some changes to the wiki and post them here too before I merge.

So, was the decision about the black and mypy dependencies to install them in the container or are we having a separate dev requirements.txt file?

I went with having three groups:

  • main dependencies: those that are needed on the servers
  • debug dependencies: those that are needed in our django docker container for local development (in addition to the main dependencies)
  • dev dependencies: those that aren't ever needed in the container

All three groups are defined in the pyproject.toml file.

@dchiller
Copy link
Contributor Author

dchiller commented May 2, 2024

Ok, for additional documentation, I've:

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.

typed_ast is EOL
4 participants