[Fixes #13859] Drop requirements.txt and setup.cfg #13860
Conversation
Summary of ChangesHello @giohappy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the project's dependency management by migrating from Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes the project's dependency management by migrating from requirements.txt and setup.cfg to pyproject.toml. This is a positive change that aligns with current Python packaging standards. My review has identified a critical issue where a dependency was dropped during the migration, which could break the application. I've also included some recommendations to improve the Dockerfile structure and the organization of dependencies within pyproject.toml for better maintainability and adherence to best practices.
| dependencies = [ | ||
| "Pillow==11.3.0", | ||
| "lxml==5.2.1", | ||
| "psycopg2==2.9.9", | ||
| "Django==5.2.9", | ||
| # Other | ||
| "beautifulsoup4==4.12.3", | ||
| "hyperlink==21.0.0", | ||
| "urllib3==1.26.18", | ||
| "Paver==1.3.4", | ||
| "python-slugify==8.0.4", | ||
| "decorator==5.1.1", | ||
| "celery==5.4.0", | ||
| "tqdm==4.66.4", | ||
| "Deprecated==1.2.14", | ||
| "jsonschema==4.22.0", | ||
| "schema==0.7.7", | ||
| "rdflib==6.3.2", | ||
| "smart_open==7.0.4", | ||
| "PyMuPDF==1.26.3", | ||
| "defusedxml==0.7.1", | ||
| "zipstream-ng==1.8.0", | ||
| # Django Apps | ||
| "django-allauth==0.63.6", | ||
| "django-appconf==1.0.6", | ||
| "django-filter==24.2", | ||
| "django-imagekit==5.0.0", | ||
| "django-taggit==5.0.1", | ||
| "django-markdownify==0.9.5", | ||
| "django-mptt==0.16.0", | ||
| "django-treebeard==4.7.1", | ||
| "django-guardian<2.4.1", | ||
| "django-downloadview==2.3.0", | ||
| "django-polymorphic==3.1.0", | ||
| "django-tastypie<0.15.0", | ||
| "django-tinymce==3.7.1", | ||
| "django-uuid-upload-path==1.0.0", | ||
| "django-widget-tweaks==1.5.0", | ||
| "django-sequences==3.0", | ||
| "oauthlib==3.3.1", | ||
| "pyjwt==2.8.0", | ||
| "redis==6.2.0", | ||
| # geopython dependencies | ||
| "pycsw @ git+https://github.com/geopython/pycsw.git@3.0.0-beta1", | ||
| "pyproj<3.8.0", | ||
| "OWSLib==0.34.1", | ||
| "SQLAlchemy==2.0.43", | ||
| "Shapely==2.1.2", | ||
| "mercantile==1.2.1", | ||
| "numpy==1.26.*", | ||
| "Jinja2==3.1.6", | ||
|
|
||
| # Apps with packages provided in GeoNode's PPA on Launchpad. | ||
|
|
||
| # Django Apps | ||
| "dj-database-url==2.1.0", | ||
| "dj-pagination==2.5.0", | ||
| "django-select2==8.4.3", | ||
| "django-forms-bootstrap<=3.1.0", | ||
| "django-autocomplete-light==3.11.0", | ||
| "django-invitations<2.1.1", | ||
| "django-recaptcha==4.0.0", | ||
|
|
||
| # REST | ||
| "djangorestframework==3.15.2", | ||
| "djangorestframework-gis==1.2.0", | ||
| "djangorestframework-guardian==0.3.0", | ||
| "drf-extensions==0.7.1", | ||
| "drf-spectacular==0.27.2", | ||
| "geonode-dynamic-rest==2.3.0.1", | ||
| "geonode-pinax-notifications==6.0.0.3", | ||
|
|
||
| # GeoNode org maintained apps. | ||
| # django-geonode-mapstore-client==4.0.5 | ||
| "django_geonode_mapstore_client @ git+https://github.com/GeoNode/geonode-mapstore-client.git@master", | ||
| "django-avatar==8.0.1", | ||
| "geonode-oauth-toolkit==2.2.2.2", | ||
| "geonode-announcements==2.0.2.2", | ||
| "django-activity-stream==2.0.0", | ||
| "gn-arcrest==10.5.6", | ||
| "geoserver-restconfig==2.0.16", | ||
| "gn-gsimporter==2.0.4", | ||
| "gisdata==0.5.4", | ||
| # importer dependencies | ||
| # blocked to <80 to avoid warning during the runtime for with py312 related to the polymorphic model | ||
| "setuptools>=59,<80", | ||
| "pdok-geopackage-validator==0.14.4", | ||
| "geonode-django-dynamic-model==0.4.0", | ||
| # datetimepicker widget | ||
| "django-bootstrap3-datetimepicker-2==2.8.3", | ||
| # storage manager dependencies | ||
| "django-storages==1.14.3", | ||
| "dropbox==11.36.2", | ||
| "google-cloud-storage==2.16.0", | ||
| "boto3==1.39.3", | ||
| # Django Caches | ||
| "whitenoise==6.6.0", | ||
| # Contribs | ||
| "xmltodict<0.13.1", | ||
| "inflection>=0.4.0", | ||
| "mock<6.0.0", | ||
| "python-dateutil==2.9.0.post0", | ||
| "pytz==2024.1", | ||
| "requests==2.32.5", | ||
| "timeout-decorator==0.5.0", | ||
| "pylibmc==1.6.3", | ||
| "sherlock==0.4.1", | ||
| "GDAL==3.8.4", | ||
| "psutil==5.9.8", | ||
| "django-cors-headers==4.9.0", | ||
| "django-user-agents==0.4.0", | ||
| # production | ||
| "uWSGI==2.0.30", | ||
| "docker==7.0.0", | ||
| "invoke==2.2.0", | ||
| # tests | ||
| "coverage==7.5.1", | ||
| "requests-toolbelt==1.0.0", | ||
| "flake8==7.3.0", | ||
| "black==24.4.2", | ||
| "pytest==8.4.2", | ||
| "pytest-bdd==7.1.2", | ||
| "splinter==0.21.0", | ||
| "pytest-splinter==3.3.2", | ||
| "pytest-django==4.8.0", | ||
| "pixelmatch==0.3.0", | ||
| "factory-boy==3.3.0", | ||
| "flaky==3.8.1", | ||
| "selenium>=4.15.1,<5.0.0", | ||
| "selenium-requests==2.0.4", | ||
| "webdriver_manager==4.0.1", | ||
| # Security and audit | ||
| "cryptography==46.0.3", | ||
| "jwcrypto>=1.5.6", | ||
| ] |
There was a problem hiding this comment.
The dependency django-modeltranslation was present in the old setup.cfg but is missing from the dependencies list in this file. The project's settings in geonode/settings.py still contain configurations for MODELTRANSLATION (e.g., MODELTRANSLATION_LANGUAGES), which indicates this dependency is still required. Please add it back to avoid breaking the application. The version in setup.cfg was 0.18.13. It would be best to add it within the "Django Apps" section for consistency.
There was a problem hiding this comment.
@mattiagiupponi we're not relying on model translations anymore. We can remove those settings in a separate issue.
Dockerfile
Outdated
| RUN . /usr/src/venv/bin/activate | ||
| RUN yes w | pip install --src /usr/src -r requirements.txt &&\ | ||
| yes w | pip install -e . | ||
| RUN yes w | pip install -e . |
There was a problem hiding this comment.
The RUN . /usr/src/venv/bin/activate command has no effect on the subsequent RUN command, as each RUN instruction executes in a separate shell. To ensure the virtual environment is activated for the pip install command and to reduce the number of layers in the Docker image, these commands should be combined into a single RUN instruction.
RUN . /usr/src/venv/bin/activate && yes w | pip install -e .
There was a problem hiding this comment.
We don't need the env activation @mattiagiupponi . We already dropped it inside the project; we should drop it here too. The environment is already activated inside the base image.
pyproject.toml
Outdated
| # tests | ||
| "coverage==7.5.1", | ||
| "requests-toolbelt==1.0.0", | ||
| "flake8==7.3.0", | ||
| "black==24.4.2", | ||
| "pytest==8.4.2", | ||
| "pytest-bdd==7.1.2", | ||
| "splinter==0.21.0", | ||
| "pytest-splinter==3.3.2", | ||
| "pytest-django==4.8.0", | ||
| "pixelmatch==0.3.0", | ||
| "factory-boy==3.3.0", | ||
| "flaky==3.8.1", | ||
| "selenium>=4.15.1,<5.0.0", | ||
| "selenium-requests==2.0.4", | ||
| "webdriver_manager==4.0.1", | ||
| # Security and audit | ||
| "cryptography==46.0.3", | ||
| "jwcrypto>=1.5.6", | ||
| ] | ||
|
|
||
| [project.optional-dependencies] | ||
| dev = [ | ||
| "pytest>=7.0", | ||
| "flake8", | ||
| "black", | ||
| "coverage", | ||
| ] |
There was a problem hiding this comment.
There are a couple of issues with how testing and development dependencies are organized:
- Test dependencies in main dependencies: Dependencies for testing (lines 142-156) are included in the main
[project.dependencies]list. These should be moved to an optional dependency group (e.g.,[project.optional-dependencies.test]) to avoid installing them in a production environment. - Conflicting
devdependencies: The[project.optional-dependencies.dev]group (lines 163-168) specifies unpinned versions of tools that are already pinned in the maindependencieslist (e.g.,pytest,flake8). This can cause version conflicts.
It's recommended to move all test and development dependencies into one or more optional groups (like test and dev) and ensure version specifications are consistent.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13860 +/- ##
===========================================
+ Coverage 52.15% 74.18% +22.03%
===========================================
Files 944 944
Lines 56335 56401 +66
Branches 7626 7632 +6
===========================================
+ Hits 29380 41843 +12463
+ Misses 25588 12879 -12709
- Partials 1367 1679 +312 🚀 New features to boost your workflow:
|
No description provided.