PTV-1888 #192
PTV-1888 #192
Conversation
…ncies [PTV-1888] python 3.11 no longer supports debian "buster".
being nice to developers
they were broken for a few reasons: ordering of fileutil & app client, invalid time comparisons, and an async workaround for hypothesis is no longer necessary
move api and development docs from README to separate, linked docs; add docs for dev tools and dockerfiles
|
Hi @bio-boris, I'm working through the code quality reports, and one big area is markdown linting issues. These are reported by codacy using "remark-lint". A few years ago they switched to "markdownlint" as the preferred markdown linting tool. Markdownlint is a much more popular tool than remark, as well. |
|
Also, the sonarcloud error report for running a container as root. AFAIK this is a requirement for running a container in GHA (some people have tried to have a non-root user in the container match whatever user is used in GHA, but as far as I can tell this is not officially supported). |
I have changed it
|
|
Thanks @bio-boris |
|
The SonarCloud is not a strict requirement for passing a PR review. It is for informational purposes to help us see if we did something wrong. I have marked it as Reviewed and now it is passing. |
|
I have triggered a re-run of codacy and it now passes. However, I noticed that you are using requirements.txt and requirements-dev Maybe we should instead get rid of requirements alltogether and switch to the PipFile? In the pipfile we can have both regular and dev dependencies |
.devcontainer/Dockerfile
Outdated
| WORKDIR /app | ||
|
|
||
| # Standard simplified python setup. | ||
| COPY ./requirements.txt /app/requirements.txt |
There was a problem hiding this comment.
Maybe we should use pipfile instead?
There was a problem hiding this comment.
The existing documentation says that the requirements.txt file is the one that is used, and that the Pipfile is there and (possibly) maintained for those who like to use it for development.
Personally, I have never seen a backup package manager in a repo like this, and wish it weren't so. In some JS codebases we might have had npm and bower in the same repo, but that was a transition from an old package manager to a newer one.
I'd be ok with Pipfile, though I think if we switch the package manager Poetry would be a better choice than pipenv.
IMO It would be nice for KBase to support just 2 Python package managers - plain pip with requirements.txt (since it is so common) for legacy projects (and it is the one supported by kb-sdk), and Poetry for new repos.
| # Don't worry, our main interaction with the container is through the | ||
| # VSC terminal, which for a devcontainer opens a shell within the | ||
| # container. | ||
| command: /bin/sh -c "while sleep 1000; do :; done" |
There was a problem hiding this comment.
Is it possible to also open a debug server with this somehow and connect your IDE debugger into this with VSCODE? I believe it can be done in pycharm. Just wondering.
There was a problem hiding this comment.
Yes, pretty sure, although I haven't tried. When using a devcontainer you are actually running VSC in the container, so it is like running on whatever the platform is - in this case debian.
Unfortunately jetbrains is still catching up with devcontainers. They have initial support (and they have other similar things that are not as clean and smooth), but it is not very complete so can't be used for real work.
I typically run the service in development mode, and bang away at endpoints from an http tool, and then the front end, and use either a terminal in the devcontainer or the host tools to run tests.
In later work (the next PR), I have used these tools, esp. mypy, more extensively and will have a few changes.
I find it effective to have cli tool support via a tool container and in the devcontainer, and to have those same tools available via VSC (or pycharm) plugins. It is a some work to set up, a pain in a codebase that is big and has not used them, and not insignificant maintenance, but I think that is the price for Python development these days.
(Don't get me started on all the extra work required to keep a Python codebase healthy; but I think ruff will be a good substitute for some of those tools and others not included here.)
There was a problem hiding this comment.
Looks like env doesn't work on my machine
Upgraded my docker-compose still doesn't work
Docker Compose version 2.21.0
environment:
KB_DEPLOYMENT_CONFIG: ./deployment/conf/local.cfg
FILE_LIFETIME: 90
might work
| bash run_tests.sh | ||
| - name: Checkout the repo | ||
| uses: actions/checkout@v3 | ||
|
|
docs/api.md
Outdated
|
|
||
| - all paths should be specified treating the user's home directory as root | ||
|
|
||
| - The url base, in an actualy deployment, will be: |
There was a problem hiding this comment.
Fixed this and about a half dozen other typos.
|
|
||
| ## Test Service | ||
|
|
||
| `GET /test-service` |
There was a problem hiding this comment.
This current serves as the /status endpoint. Maybe we can have 2 routes point to the same status function?
There was a problem hiding this comment.
Yeah, we could, and should, but perhaps not in this PR.
| ### Running Tools | ||
|
|
||
| All of the tools available for running from the host via docker are also directly | ||
| available within the devcontainer. I still often run them from a host termianl anyway, as |
There was a problem hiding this comment.
Yeah, fixing typos in that file. I didn't have the spellchecker running so those fingers do slip...
| - needs to copy all service files | ||
| - does not include development Python dependencies | ||
| - has the production entrypoint | ||
| - `./.devcontainer/Dockerfile` |
There was a problem hiding this comment.
I think we could probably use one dockerfile and I think we could copy in the helper scripts and alternate endpoints in there as well. The entrypoint could also install required dev dependencies as well.
There was a problem hiding this comment.
Ugh, I was hoping not to have to deal with multipurposing a single image.
What about using a Dockerfile include for the main setup? Basically the base could be from the base image up to the installation of Python service dependencies. This basically leaves the main service image to set up the entrypoint, and the development images to install the dev dependencies and have separate default entrypoints?
There was a problem hiding this comment.
Well, I had read about Dockerfile INCLUDE, and not tried it ... I now have, and it isn't working (but perhaps that is just me, atm.) It also relies upon 3rd party extensions. The Docker team does not want to implement it, despite passionate and salty pleas: moby/moby#735
There was a problem hiding this comment.
Alternative way is
# Base stage
FROM python:3.11 AS base
# Install shared dependencies, set up common environment...
# Production stage
FROM base AS production
# Copy service files, set up production-specific stuff...
# Development stage
FROM base AS development
# Set up dev-specific stuff...
docker build --target base -t myimage:base .
docker build --target production -t myimage:production .
docker build --target development -t myimage:development .
Could also use --build-args as well
There was a problem hiding this comment.
Nice!
I'll have a crack at this, though I'd do it using docker compose files.
There was a problem hiding this comment.
Oh, and another question -
F. Wondering why the image installs htop and wget -- as conveniences for poking around a container? If they aren't used I'd remove them and only install in the dev target.
There was a problem hiding this comment.
okay, @bio-boris , the Dockerfile and related changes are up now. I also took the opportunity to fold in some tool (black, isort, autoflake, flake8, mypy) improvements.
There was a problem hiding this comment.
3.200 Package wget is not available, but is referred to by another package.
3.200 This may mean that the package is missing, has been obsoleted, or
3.200 is only available from another source
3.200
3.202 E: Version '1.21.3-1+b2' for 'wget' was not found
------
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
Dockerfile:26
--------------------
25 | # Install OS dependencies required by or nice-to-have in a development image
26 | >>> RUN apt-get update && \
27 | >>> apt-get install -y --no-install-recommends htop=3.2.2-2 wget=1.21.3-1+b2 git=1:2.39.2-1.1 openssh-client=1:9.2p1-2 && \
28 | >>> apt-get clean && \
29 | >>> rm -rf /var/lib/apt/lists/*
30 | # Install Python dependencies require by development tools (cli tools and devcontainer)
--------------------
ERROR: failed to solve: process "/bin/sh -c apt-get update && apt-get install -y --no-install-recommends htop=3.2.2-2 wget=1.21.3-1+b2 git=1:2.39.2-1.1 openssh-client=1:9.2p1-2 && apt-get clean && rm -rf /var/lib/apt/lists/*" did not complete successfully: exit code: 100
docker build . --target dev
There was a problem hiding this comment.
Its very convenient to have wget and curl in all containers. They are very small and are very useful and also useful, especially when we get network connectivity issues. I'd prefer not to remove that from the main image, as we can always install htop. In the distant, but not too distant future, we can use k8 debug containers, but until then it saves a lot of trouble by having them directly in the container. In fact, we could even use DockerSlim once we can use debug containers.
There was a problem hiding this comment.
Docs about some of those file are in our confluence
****************************
ACL And File Manager CronJob
****************************
/root/bulk/acl_manager/ contains the required globus.cfg and python dependencies to run *prune_and_delete.py*
The threshold to delete files and acls is defined as the constant THRESHOLD_DAYS, (currently set to 60)
If the modfied time of the user's directory is > THRESHOLD_DAYS, the ACL is marked as old, and deleted from globus, as well as the entire user's directory on */dtn/disk0/bulk/<username>*
The script should be run in the /root/bulk/acl_manager directory in order to access the *globus.cfg* and to consistently save logs in the same place.
See `here. <prune_and_delete.py>`_
****************************
ISSUES
****************************
* run /opt/kb-ftp-api/refresh_token.py to refresh token manually, otherwise this is run in a cronojb
* if it fails, uncomment the code inside to request a new token and rerun it
* if the staging service token fails, copy over the "REFRESH TOKENS" only into the appropriate values into the staging services globus.cfg file.
*
requirements.txt
Outdated
| defusedxml==0.7.1 | ||
| frozendict==2.3.8 | ||
| globus-sdk==1.6.1 | ||
| hypothesis==6.81.1 | ||
| globus-sdk==3.28.0 |
There was a problem hiding this comment.
I don't think we can upgrade globus
There was a problem hiding this comment.
Ok, tests pass either way, I'll revert to the old version.
Uses multi-stage build to create a dev stage and production stage; removed other Dockerfiles, adjusted docker-compose and other things.
includes aligning black to the previous line length of 100, aligns isort to black (otherwise they fight each other), set up mypy and autoflake, flake8 doesn't use pyproject
Hold back globus_sdk
|
SonarCloud Quality Gate failed.
|
|
|
||
| # Nice to set this up for all future python code being run. | ||
| # We set the root of the repo as the python path. | ||
| export PYTHONPATH="${PWD}" |
There was a problem hiding this comment.
I am confused as to why this bash script is necessary instead of specifying the commands in the docker-compose.yaml file or the docker-compose binary.
| "tamasfe.even-better-toml", | ||
| "mhutchie.git-graph", | ||
| "ms-python.black-formatter", | ||
| "ms-python.flake8", |
There was a problem hiding this comment.
I suggest moving to Ruff -- it replaces most/all flake8 packages as well as adding its own rules, has an auto fix setting, and is much quicker than flake8. There's a VSCode integration for it: https://github.com/astral-sh/ruff-vscode
There was a problem hiding this comment.
On second thoughts, having finished going through the PR, leave Ruff for another PR. Not worth waiting to merge this one for a "nice to have".
| @@ -0,0 +1,7 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
is there a reason why this doesn't use /usr/bin/env bash like the other scripts?
| This document provides an overview of two different but related development approaches - | ||
| devcontainers (via Visual Studio Code), and command line tools via a Docker container. |
There was a problem hiding this comment.
this is great! I've also been using vscode within running docker containers using VSCode's Docker extension. We should really have some sort of skill-share thing with anyone who does development for KBase as most people don't take advantage of what VSCode can do because it can be a PITA to set up.
PR for PTV-1888
The goal of PTV-1888 is to add development tools to the staging service, in preparation for a future PR (which will follow on the heels of this one) which fixes and improves support for file uploads of up to 5GB.
This PR contains the following changes:
./developmentdevcontainerusable with Visual Studio Code located in./devcontainerrequirements.txtfor the deployment runtime, andrequirements-dev.txtcontaining additional dependencies for development and testing.README.md.Development Tools
The development tools are designed to be run in a docker container, which may be a bespoke container defined
./development/Dockerfileand controlled by a script./development/scripts/run, or may be the aforementioneddevcontainer.These tools include
mypy,black,isort, and thepytest-based tests.devcontainerDevcontainers are a relatively recent addition to the developer toolkit. Visual Studio Code (VSC) is already supported by this repo, so this can be seen as an addition to that support. The devcontainer has its own Dockerfile. For those unfamiliar with devcontainers, it is essentially a method for running VSC inside a container, and that container is configured to support the programming language and environment of choice. This is a very effective technique, as it replicates the precise system, system dependencies, python version, and python dependencies.
Splitting Requirements
As the number of development support tools expands to cover current practices, the entanglement of runtime and development-time dependencies becomes more serious. Therefore, the dependencies were simply split into "requirements.txt" and "requirements-dev.txt". The production image installs only "requirements.txt", and the devcontainer and development tools images also install the requirements-dev.txt.
Update Dependencies
It is always good to take the opportunity to ensure that dependencies are up-to-date, to reduce the diff to dependencies in subsequence pull requests. In this case it was triggered by two other changes - the need to add new dependencies to support development tools, and the need to update the images due to a problem with the current Dockerfiles.
There is nothing else notable about the dependency updates.
Dockerfile Update
I can briefly describe the Dockerfile issue. It appears that the Python base image dropped support for recent Python versions and the Debian "buster" releases. The effect of this was that the image would no longer build. So the Dockerfile was updated to use the base image
python:3.11.5-slim-bookworm. This is a minor Python version bump from3.11.4, a shift from Debianbustertobookworm, and an update of all the OS-level dependencies installed in the Dockerfile.Fixed Tests
There were two types of test fixes required.
First, the tests had used an small patch for running
hypothesisin asynchronous tests. Some tests seemed to occasionally fail when tearing down the file system utility used to prepare a directory for tests to use. There seemed to be a race condition, and sometimes a file which was assumed to be deleted was not, and the operation to remove a directory used for test data would fail. I had assumed this was in the "custom" async adapter for the hypothesis library. It was during that investigation that I discovered that the custom adapter was no longer necessary, as hypothesis now works fine with asyncio-based tests. However, after this change, the test failures persisted, and I determined that there should not be any race conditions, as the async tests should be sequenced (although there is always a doubt that concurrency will creep in). The next conjecture was that it was due to the interaction between the async client mock and the file utility. It ends up that the async client mock needs should be shut down before the file utility cleanup code, and thus the async client should be wrapped in the file utility . Applying this change to all such tests fixed the problem.The second test problem was due to a time comparison for the modifiction type of file metadata. The test asserts that the when creates a file, the modification type should be earlier than any time measurement taken afterwards. However, sometimes this assertion failed! The reason? It is possible that the modification time obtained for a file may have a rough precision - in this case it was 1 second, although research showed that in windows it may be 2 seconds. In such a case, the modification time may be rounded to the nearest second, which may end up being the next second. Since the elapsed wall time between the file creation and the test code that generates the time comparison is so short, sub-second for sure, it is possible for the file creation time to be greater than the comparison time. This is not breaking the laws of sequential programming - it is just due to the modification time being rounded to the next second.
The solution was to only look at the absolute difference between the two times, and ensure that it is less than 1 second.
BTW this file modification precision issue was noticed only on my host machine - it did not replicate on GHA for example. This is probably due to the underling virtual machine used on a Rancher Desktop on macOS.
Markdown Work
To support the new development tools, I added additional documentation. Since I now relied on being able to edit markdown, I also added markdownlint support. This in turn made it convenient fix the markdown linting issues in all markdown files. There are not many, so it did not take much time.
Markdown linting is not integrated into the development toolchain, just used environmentally in VSC. Support is in the form of a VSC plugin in the devocontainer configuration, and a
.markdownlint.yamlconfiguration file.Extracted API Documentation
More time-consuming was extracting the API documention from the top-level README.md. I was editing the top level README in order to reflect the new development tools, and found that the length of the file was onerous to deal with - it was over 1K lines. And the documentation format was difficult to read, and did not follow any kind of standard format. I hope the improvements I made will improve that situation, and I didn't quite complete it as some of the later code examples are somewhat difficult to follow. I did reformat them somewhat, but they are still cumbersome to read.