-
-
Notifications
You must be signed in to change notification settings - Fork 275
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
build: update backend dependency management to pdm + dockerfile #5184
build: update backend dependency management to pdm + dockerfile #5184
Conversation
This is only a partial solution, as there are multiple references to requirements.txt throughout the repo. python-venv is also used, but is now redundant with using pdm. I have a commit on this branch that I have not pushed yet, removing requirements.txt and modifying any docs / workflows with reference to it (14 files). |
Thanks for the PR @swoodcoc. I think it will be easier to review if you push it and maybe you can also describe what the user needs to do to set up the project (like instead of |
No worries, I'll add it now. I have updated all references to requirements.txt and using venv that I could find in the repo. As for the benefit:
|
c0efd43
to
660b261
Compare
So it actually looks like you can't pip install dependencies directly from a pyproject.toml (I have never had to before). I wanted to keep the edits to CI etc simple by pip installing the already solved dependency list. To achieve this we can
I will update. |
Everything should be complete to the best of my knowledge. There seem to be quite a few Dockerfiles for the backend to maintain - I updated them all, as I wasn't sure which ones are redundant or still used by external processes. |
Also on a side note: the Dockerfile using The dependency install worked without issue (no conflicts) using Python 3.9, so I feel like both could be updated to that anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some dependencies failed to install while setting up locally without docker but no such issues while using docker, other than that this looks good to me. Also, we may need to update the doc mentioning that the python version set in pyproject.toml
should be preinstalled on the system before setting up locally without docker.
Thanks for the feedback, I'm more than happy to go through the docs and update as requested. As for the package installs, are you able to I imagine the reason install failed is because of missing libraries on your Linux system, such as A local install requires all the libs packaged into the docker image to be installed on your machine: Lines 11 to 22 in 4757148
The Dockerfile above uses Alpine, so the packages would be named differently for example on a Debian based distro (e.g. Ubuntu). |
675445e
to
0818fb6
Compare
I rebased and added the suggested changes to the docs. |
0818fb6
to
4e82e67
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Related to #5705 |
I could update this PR and rebase with the current |
Thank you, @spwoodcock . We are currently swamped and unable to review your upgrades immediately. Can you please give us a week or two to get |
@ramyaragupathy @Aadesh-Baral let us discuss this in our next meeting and go over timelines and effort involved. |
Of course @eternaltyro, no rush at all! Send me a message over any platform when you have time for this 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spwoodcock I have left some comments here - mostly prompts and reminders to discuss these. Everything else looks good. This is a significant quality upgrade to the Backend infrastructure. Thank you very much for taking the time and effort to do this.
@Aadesh-Baral Let's discuss this once we meet next week. Please hold merging even after current merge conflicts have been resolved. cc: @dakotabenjamin @ramyaragupathy
Dockerfile
Outdated
LABEL hotosm.org.maintainer="HOT Sysadmin <[email protected]>" | ||
LABEL hotosm.org.description="Builds docker image for TM Backend" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand MAINTAINER label is deprecated and we can simply use LABELS for it. But what's the reason for using hotosm.org.maintainer as key? Docker suggests tld.domain.sub.maintainer but I'm not sure if it is standard. If it is, we can switch this to org.hotosm.maintainer.
Documentation about this is sparse though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there are any standards around label keys as far as I know, we can definitely change it 👍
I'll try to find time to revise this PR soon (particularly the dockerfile). There are a few refinements I could include, plus Python 3.10. |
Thank you, @spwoodcock . Let me know once you do, and I'll give it another review. |
I should get around to updating this at the weekend - got a pretty full work week ahead 👍 |
f456630
to
3420d04
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
I just rebased with develop and added the The file can be deleted after merging #5642 and updating the Ready to merge 👍 |
Merged. All good so far. Thanks @spwoodcock for your time and labour. It's much appreciated 🙏🏼 |
The earlier PR #5184 switched the default TM_LOG_DIR from `logs` to `/home/appuser` which is not present in circleCI. Switched it out for /tmp/logs to make tests work.
PEP 582 has been rejected! - https://discuss.python.org/t/pep-582-python-local-packages-directory/963/430 We will continue to use venv for now until our move to containers is done. |
Yes it has, but that doesn't mean it's useless. PDM defaults to venv based installs. PEP582 support is optional, but is a much better alternative IMO. The rejection shouldn't affect this PR. |
I agree. But we have to play nice with standards and conventions that have buy in from the community also.
Agreed. I am only concerned about PEP582; the rest of the features PDM brings are still useful. Going forward, we will use venv, preferably managed by PDM and conforming to PDM defaults. All this is moot anyway with containers taking over. |
Closes #4642
Looks like this issue has been open for a while.
Great suggestion by @frafra to use PDM. Thank you.
Notes: