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

Accso delivery for pytroll image comparison tests #2

Merged
merged 16 commits into from
Jan 24, 2025

Conversation

gerritholl
Copy link
Member

@gerritholl gerritholl commented Oct 11, 2024

This PR adds pytroll image comparison tests as delivered by Accso, developed as part of a contract with DWD.

Details can be found in the README file that is part of this commit.

  • Check that docker returns error when something fails (currently it's claiming success and linking to old results, when it actually failed).
  • Put reference images on zenodo (done)
  • ... and take them from there.
  • Check if ansible copy has a mode to only copy files that have changed
  • Make a package with pyproject.toml
  • Add at least a start of unit tests
  • Add a start of CI
  • Would it be possible to "freeze" all the apt-get install steps so they don't need to be rerun every time inside the container?
  • It seems old docker run files are not cleaned up (unlike the data, which are).
  • Why does it sometimes hang at Cloning into '/app/repository'? Can we prevent it?
  • Related to hanging, but not only: Can we automatically kill a docker container after a certain timeout?
  • It always links to the latest test result, should link directly to the relevant test result, or older results get confusing
  • Come up with a naming scheme for reference images and use this consistently, and/or maybe add symlinks after copying?

@gerritholl
Copy link
Member Author

Corresponding PR in Satpy: pytroll/satpy#2912

Comment on lines 16 to 19
satellite = "GOES17"
filenames = glob(f'{ext_data_path}/satellite_data/{satellite}/*.nc')

scn = Scene(reader='abi_l1b', filenames=filenames)
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to make this more flexible.

Copy link
Member

Choose a reason for hiding this comment

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

indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall we defer this to a later PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need this file?

Comment on lines 55 to 57
filenames = glob(f'{ext_data_path}/satellite_data/{satellite}/*.nc')

scn = Scene(reader='abi_l1b', filenames=filenames)
Copy link
Member Author

Choose a reason for hiding this comment

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

We will need to make this more generic.

Copy link
Member

Choose a reason for hiding this comment

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

yes. if we add a third column in the table with the reader name, that should be fine

Translate comments and strings from German to English.
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Not too bad!

ansible/files/systemd/bildabgleich.service Outdated Show resolved Hide resolved
ansible/inventory.ini Outdated Show resolved Hide resolved
ansible/roles/add_user/tasks/main.yml Outdated Show resolved Hide resolved
ansible/roles/certbot_nginx/CODE_OF_CONDUCT.md Outdated Show resolved Hide resolved
ansible/templates/nginx/sites-available/bildabgleich Outdated Show resolved Hide resolved
Comment on lines 16 to 19
satellite = "GOES17"
filenames = glob(f'{ext_data_path}/satellite_data/{satellite}/*.nc')

scn = Scene(reader='abi_l1b', filenames=filenames)
Copy link
Member

Choose a reason for hiding this comment

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

indeed.

Comment on lines 55 to 57
filenames = glob(f'{ext_data_path}/satellite_data/{satellite}/*.nc')

scn = Scene(reader='abi_l1b', filenames=filenames)
Copy link
Member

Choose a reason for hiding this comment

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

yes. if we add a third column in the table with the reader name, that should be fine

serverLogic/server.py Outdated Show resolved Hide resolved
serverLogic/start_server.sh Outdated Show resolved Hide resolved
mraspaud and others added 4 commits November 18, 2024 17:03
@gerritholl
Copy link
Member Author

A test with actual data fails on image-test:

Nov 20 08:56:32 image-test.pytroll.ewc start_server.sh[106787]: INFO:server:Starting to clone and test the repository pytroll/satpy
Nov 20 08:58:12 image-test.pytroll.ewc start_server.sh[106787]: Directory /home/imagetester/pull_request_feature-multifile-handler successfully emptied.
Nov 20 08:58:12 image-test.pytroll.ewc start_server.sh[106787]: Directory /home/imagetester/pull_request_feature-multifile-handler newly created.
Nov 20 08:58:12 image-test.pytroll.ewc systemd[1]: Stopping image-comparison-server...
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: State 'final-sigterm' timed out. Killing.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 106781 (gunicorn) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 106787 (gunicorn) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112208 (docker) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112187 (gunicorn) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112209 (n/a) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112214 (n/a) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112216 (n/a) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112217 (n/a) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112218 (n/a) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Killing process 112219 (n/a) with signal SIGKILL.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Failed with result 'timeout'.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: Stopped image-comparison-server.
Nov 20 08:58:17 image-test.pytroll.ewc systemd[1]: image-comparison.service: Consumed 1.150s CPU time.

@gerritholl
Copy link
Member Author

From the docker log:

$ tail /home/imagetester/pull_request_feature-multifile-handler/output.log
  Downloading zipp-3.21.0-py3-none-any.whl (9.6 kB)
Collecting locket
  Downloading locket-1.0.0-py2.py3-none-any.whl (4.4 kB)
Installing collected packages: parse, zipp, toolz, tomli, six, pyyaml, pluggy, Pillow, packaging, numpy, locket, iniconfig, fsspec, exceptiongroup, cloudpickle, click, certifi, pytest, partd, parse-type, opencv-python, importlib-metadata, h5py, cftime, netcdf4, h5netcdf, dask, behave
Successfully installed Pillow-11.0.0 behave-1.2.6 certifi-2024.8.30 cftime-1.6.4.post1 click-8.1.7 cloudpickle-3.1.0 dask-2024.11.2 exceptiongroup-1.2.2 fsspec-2024.10.0 h5netcdf-1.4.1 h5py-3.12.1 importlib-metadata-8.5.0 iniconfig-2.0.0 locket-1.0.0 netcdf4-1.7.2 numpy-2.1.3 opencv-python-4.10.0.84 packaging-24.2 parse-1.20.2 parse-type-0.6.4 partd-1.4.2 pluggy-1.5.0 pytest-8.3.3 pyyaml-6.0.2 six-1.16.0 tomli-2.1.0 toolz-1.0.0 zipp-3.21.0

[notice] A new release of pip is available: 23.0.1 -> 24.3.1
[notice] To update, run: python3 -m pip install --upgrade pip
Cloning into '/app/repository'...
fatal: Remote branch feature-multifile-handler not found in upstream origin

@gerritholl
Copy link
Member Author

gerritholl commented Nov 20, 2024

Looks like it's searching for the branch in upstream and not in the fork where the pull request originates?

@gerritholl
Copy link
Member Author

After increasing the logging amount for debugging purposes, rerunning fails with a new error message:

Nov 20 09:18:09 image-test.pytroll.ewc start_server.sh[118722]: INFO:server:Starting to clone and test the repository pytroll/satpy
Nov 20 09:18:10 image-test.pytroll.ewc start_server.sh[118769]: docker: Error response from daemon: Conflict. The container name "/clone-repo-image" is already in use by container "96379bfa2a82d5be90fdd895071f4a2491df31ecf323c714c8dade8b9194e969". You have to remove (or rename) that container to be able to reuse that name.
Nov 20 09:18:10 image-test.pytroll.ewc start_server.sh[118769]: See 'docker run --help'.
Nov 20 09:18:10 image-test.pytroll.ewc start_server.sh[118722]: ERROR:container_utils:Error while cloning the repository: Command '['docker', 'run', '--name', 'clone-repo-image', '-v', '/home/imagetester/pull_request_feature-multifile-handler:/app', '-v', '/home/imagetester/pytroll-image-comparison-tests/data:/app/ext_data', 'python:3.10-slim', 'bash', '-c', 'touch /app/output.log && apt-get update >> /app/output.log 2>&1 && apt-get install -y git libgl1-mesa-glx libglib2.0-0 python3-venv >> /app/output.log 2>&1 && python3 -m venv /app/venv >> /app/output.log 2>&1 && /app/venv/bin/pip install behave Pillow pytest numpy opencv-python dask netcdf4 h5netcdf >> /app/output.log 2>&1 && git clone https://[REDACTED]@github.com/pytroll/satpy.git --branch feature-multifile-handler /app/repository >> /app/output.log 2>&1 && source /app/venv/bin/activate && pip install -e /app/repository >> /app/output.log 2>&1 && source /app/venv/bin/activate && cd /app/repository/satpy/tests/behave && behave >> /app/output.log 2>&1 || true && chown -R 1005:1005 /app >> /app/output.log 2>&1']' returned non-zero exit status 125.
Nov 20 09:18:10 image-test.pytroll.ewc start_server.sh[118791]: Error response from daemon: Could not find the file /app/output.log in container clone-repo-image

Looks like there is an old hanging docker image from the previous failed job, that did not get cleaned up?

@gerritholl
Copy link
Member Author

To test the hypothesis that it's only searching for the branch in upstream and not in the fork, I created a dummy PR in pytroll/satpy#2987 based on a branch in the upstream. However, now trying to trigger the behave tests fails with:

Nov 20 09:53:44 image-test.pytroll.ewc start_server.sh[118722]: ERROR:server:Signature verification failed.

@gerritholl
Copy link
Member Author

Seems that for pytroll/satpy#2697 it did actually work in the end, despite the error message 'fatal: Remote branch feature-multifile-handler not found in upstream origin'.

It also works for pytroll/satpy#1136.

But for pytroll/satpy#2987 (where the branch is on the main fork, not the user's fork) it fails to verify the signature. Don't know why, but not critical as it works (despite the error message) for normal PRs.

@gerritholl
Copy link
Member Author

Correction. It did not work. The linked test result refers to an old test from September. It cannot have worked, because there are no reference images on the server.

So probably the message fatal: Remote branch feature-multifile-handler not found in upstream origin is the problem after all. As a secondary problem, it shouldn't claim "The testing process was executed successfully." when it was not.

@gerritholl
Copy link
Member Author

As a secondary problem, it shouldn't claim "The testing process was executed successfully." when it was not.

I guess docker run does not fail if the underlying git clone fails, and therefore subprocess.check_call does not fail either.

Clone from the fork, not from the original satpy repo.
Added pyproject.toml.  Started some first unit tests.  Did a bit of
refactoring.
Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

There are some duplications compared to pytroll/satpy#2912

behave/create_reference.py Outdated Show resolved Hide resolved
behave/features/image_comparison.feature Outdated Show resolved Hide resolved
behave/features/steps/image_comparison.py Outdated Show resolved Hide resolved
behave/modify_image.py Outdated Show resolved Hide resolved
Delete duplicate behave tests.  Already present in Satpy PR at
pytroll/satpy#2912
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

looks good, ready to merge?

@gerritholl gerritholl merged commit 97d0b20 into main Jan 24, 2025
@gerritholl gerritholl deleted the pytroll-image-comparison-tests branch January 24, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants