Skip to content
This repository has been archived by the owner on Apr 16, 2023. It is now read-only.

Automate image build #130

Merged
merged 9 commits into from
Oct 27, 2020
Merged

Automate image build #130

merged 9 commits into from
Oct 27, 2020

Conversation

davidwilby
Copy link
Contributor

@davidwilby davidwilby commented Oct 14, 2020

Resolves #111 (as discussed, using github actions)

Work in progress to build image and push to dockerhub on pushes to master.

To do:

  • check with ./.ci/local_checks.sh locally
  • rebase onto dev?

Changes to make before/on merging:

  • set correct dockerhub user (gsganden?) and image name.
  • add AWS credentials to repo secrets (AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY)
  • add Dockerhub username and token to repo secrets (DOCKERHUB_USERNAME and DOCKERHUB_TOKEN)

Pull Request Checklist

  • Pull request has been made against dev branch.
  • Pull request includes a description of the change and the reason behind it.
  • Pull request uses keywords to close relevant issues.
  • Pull request includes unit tests for any new functionality.
  • README and docs have been updated.
  • ./.ci/local_checks.sh passes locally. (The app must be running. See README.md for instructions.)

Maintainer's responsibilities:

  • _version.py has been updated.
  • CHANGELOG.md has been updated.
  • Updated app container has been pushed with current version number.
  • App container version number has been updated everywhere in README.

@davidwilby
Copy link
Contributor Author

davidwilby commented Oct 20, 2020

This gets most of the job done via .github/workflows/automate-image-build-push.yml however there is an error produced on building and running the container:

  File "/image_api/app/app.py", line 7, in <module>
    from werkzeug import secure_filename
ImportError: cannot import name 'secure_filename' from 'werkzeug' (/opt/conda/lib/python3.7/site-packages/werkzeug/__init__.py)

which seems to have been introduced since the last time the image was built, via a problem in the package flask-uploads described here: maxcountryman/flask-uploads#28

logged during the build as:

Collecting Werkzeug>=0.15
  Downloading Werkzeug-1.0.1-py2.py3-none-any.whl (298 kB)

EDIT
This is now accounted for as of 77481c7
I've also fixed a bug in the image file extension checking util with 5f7ef4a
(feel free to cherry-pick these commits if you don't want the rest of the branch)

@davidwilby davidwilby marked this pull request as ready for review October 20, 2020 16:47
@gsganden
Copy link
Collaborator

Got it, thanks!

@gsganden
Copy link
Collaborator

I think the changes GitHub is showing in autofocus/build_dataset/helpers.py and autofocus/predict/app/app.py were already in master but not in dev. Is that your understanding? If so, my bad.

Copy link
Collaborator

@gsganden gsganden 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 once we can get the checks to pass. Thanks!

Copy link
Collaborator

@gsganden gsganden left a comment

Choose a reason for hiding this comment

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

Marking as "Requested changes" until the build passes.

@sourcery-ai
Copy link

sourcery-ai bot commented Oct 26, 2020

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 2.08%.

Quality metrics Before After Change
Complexity 1.80 ⭐ 1.20 ⭐ -0.60 👍
Method Length 46.53 ⭐ 45.87 ⭐ -0.66 👍
Working memory 7.27 🙂 6.53 🙂 -0.74 👍
Quality 81.79% 83.87% 2.08% 👍
Other metrics Before After Change
Lines 242 241 -1
Changed files Quality Before Quality After Quality Change
autofocus/build_dataset/helpers.py 82.63% ⭐ 82.63% ⭐ 0.00%
autofocus/predict/app/app.py 71.44% 🙂 76.02% ⭐ 4.58% 👍
autofocus/predict/app/utils.py 93.47% ⭐ 95.55% ⭐ 2.08% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
autofocus/predict/app/app.py classify_zip 11 🙂 178 😞 9 🙂 51.65% 🙂 Try splitting into smaller methods
autofocus/predict/app/app.py classify_single 4 ⭐ 109 🙂 10 😞 63.81% 🙂 Extract out complex expressions
autofocus/build_dataset/helpers.py S3DownloadProgressPercentage.__call__ 0 47 ⭐ 11 😞 75.07% ⭐ Extract out complex expressions
autofocus/build_dataset/helpers.py has_channels_equal 0 39 ⭐ 11 😞 76.37% ⭐ Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Let us know what you think of it by mentioning @sourcery-ai in a comment.

@davidwilby
Copy link
Contributor Author

Yes, the majority of chose changes (except those in 77481c7 & 5f7ef4a) are from the master branch. However, the PR is made against dev as per the contribution instructions, but this branch davidwilby:automate-image-build is branched from master. So to make the changes relative to dev clearer, we could rebase onto dev? If not, merging this PR will merge all the changes on master into dev. Up to you.

Regarding the failing checks, it looks like this is to do with a version incompatibility in black - psf/black#1666 which comes down to python <3.6.2 being incompatible with a type hint import of typing.NoReturn in black 20.8b1 - so could either up the python version that the checks run on, or bring the black version in pip down.

In 7e5aaae I've tried bumping it up to 3.6.12, though since 3.9 is now released, you should maybe consider using 3.8 or higher.

@gsganden
Copy link
Collaborator

I'm OK with merging. Thank you!

@gsganden gsganden merged commit 184e4af into uptake:dev Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants