Predict app #121

DigitalPhilosopher · 2019-10-16T08:25:53Z

Feel free to open a PR before all of these items have been completed.

Pull Request Checklist

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, if relevant, with current version number.
App container version number has been updated in README.

- Add the abstract class Validator - Add the class PredictRequestValidator for validating single predict requests - Add fastai to requirements

- Use Validator in predict single - Add abort function to Validator

- Add PredictZipRequestValidator to module requests

- Handle file through file class - File is removed in the destructor - Use in predict_single

- Add ZipArchive to handle zips - Add predict multiple method to Predictor - Remove unused utils - Change File to set file or pathname - Change predict_zip in app

- Add docstrings for new classes - Remove unused imports

- Remove imports from __init__ - Fix circle import from File and app

gsganden · 2019-10-16T13:35:12Z

Got it, thanks! Will review at my earliest opportunity.

autofocus/predict/app/requests/Validator.py

DigitalPhilosopher · 2019-10-22T18:20:04Z

Hey @gsganden, is there anything else you would change in my PR? I would be very happy with some advice and comments on my changes. Thanks very much :)

gsganden · 2019-10-22T19:17:13Z

It might take me a little while, but I will get to it!

gsganden

Thanks for the contribution! You and I have different coding styles that we'll need to reconcile. Overall, I favor more functional approaches. I'm open to accepting PRs that are not the way I would have done it, but I also want the overall design to be unified and idiomatic. There are also some places (particularly in the file and zipfile handling) where just using Python's standard library and built-in capabilities will result in code that other Python developers will find easier to understand.

How would you like to proceed? I'd be happy for you to take another crack at it if you are willing. I could also take some of your contributions and adapt them in a more functional style when I get a chance.

gsganden · 2019-11-03T13:14:07Z

autofocus/predict/app/models/File.py

+from werkzeug import secure_filename
+
+
+class File:


We don't need our own file class -- Python has built-in ways to do these things.

Especially for our use case, I wrote the File class, so it would remove the file upon destructing the object. This removes the need to manually handle this every time while handling a file.

I think maybe the naming here is the problem, so I renamed this class to be 'TemporaryFile'.

I would like to keep this class, since the functionality here is pretty awesome and makes the handling very simple. However it is your call, if you don't see the value.

What do you think?

Could we use https://docs.python.org/3/library/tempfile.html?

gsganden · 2019-11-03T13:15:24Z

autofocus/predict/app/models/Predictor.py

+CLASSES = model.data.classes
+
+
+class Predictor:


Could we make these methods top-level functions? I don't see much benefit in having them together in a class rather than just a module, it requires another line of code for callers, and it seems to me less idiomatic Python.

Yes, I made them top-level functions now.
While developing the class I thought about adopting this class for other predictions as well. That was my thought behind this.

gsganden · 2019-11-03T13:16:18Z

autofocus/predict/app/models/ZipArchive.py

+from ..utils import allowed_file
+
+
+class ZipArchive:


I don't think we need our own ZipArchive class -- the standard library has these capabilities https://docs.python.org/3/library/zipfile.html

Here the same as for the 'File' (now 'TemporaryFile'). It adds more functionality to the build in functions and can be reused.

gsganden · 2019-11-03T13:18:22Z

autofocus/predict/app/requests/Validator.py

+ALLOWED_ZIP_FILES = {".zip"}
+
+
+class Validator(ABC):


The class structure here seems to me to add more overhead than it is worth. All we need is one function to validate predict requests, one to validate predict_multiple requests, and appropriate calls to abort.

I see your point and changed this to be a single method now.

- Rename File to temporary file

DigitalPhilosopher · 2019-11-05T13:15:37Z

Hey @gsganden I revisited the code and made it more functional and removed most of the object orientation.

However, I kept the File (now TemporaryFile) and ZipArchive as classes, as said before. If you really don't favor this approach, I would be willing to change it as well, but I think it adds a lot to the simplicity of the code and would favor to keep it. What do you think?

gsganden · 2019-11-25T02:53:40Z

autofocus/predict/app/routes/predict.py

+from ..validation.predict import validate_predict_request
+
+
+predict_route = Blueprint("predict", __name__)


I've never used Blueprint objects, but I'm intrigued. Can you point me to a resource that explains how they work?

gsganden

This is looking good. I think we can simplify it further by using the standard library tempfile and zipfile modules rather than creating custom classes. I'd also like to understand how Blueprint works if you could point me to relevant resources.

gsganden

This is looking good. I think we can simplify it further by using the standard library tempfile and zipfile modules rather than creating custom classes. I'd also like to understand how Blueprint works if you could point me to relevant resources.

Patrick Kuehn added 6 commits October 15, 2019 13:47

Add Validator for app

0d6e168

- Add the abstract class Validator - Add the class PredictRequestValidator for validating single predict requests - Add fastai to requirements

Use Validator

a0bf719

- Use Validator in predict single - Add abort function to Validator

Add Validator for zip files

7511cab

- Add PredictZipRequestValidator to module requests

Use PredictZipRequestValidator

8e04609

Add File class

75eeadd

- Handle file through file class - File is removed in the destructor - Use in predict_single

Add ZipArchive

fa5bd2b

- Add ZipArchive to handle zips - Add predict multiple method to Predictor - Remove unused utils - Change File to set file or pathname - Change predict_zip in app

DigitalPhilosopher requested a review from gsganden as a code owner October 16, 2019 08:25

Patrick Kuehn added 3 commits October 16, 2019 11:09

Add docstrings

191a6da

- Add docstrings for new classes - Remove unused imports

Fix imports

400c24d

- Remove imports from __init__ - Fix circle import from File and app

Refactor for travis ci imports and docstrings

74ed977

DigitalPhilosopher force-pushed the predict-app branch from d60aa76 to 74ed977 Compare October 16, 2019 09:51

DigitalPhilosopher mentioned this pull request Oct 17, 2019

More clarity in autofocus output #117

Closed

gsganden reviewed Oct 17, 2019

View reviewed changes

autofocus/predict/app/requests/Validator.py Outdated Show resolved Hide resolved

DigitalPhilosopher force-pushed the predict-app branch from 684e535 to 9e3ec64 Compare October 18, 2019 11:01

DigitalPhilosopher requested a review from gsganden October 18, 2019 11:04

Merge branch 'dev' into predict-app

31201ac

DigitalPhilosopher force-pushed the predict-app branch from 9e3ec64 to 31201ac Compare October 18, 2019 11:05

gsganden suggested changes Nov 3, 2019

View reviewed changes

Patrick Kuehn added 4 commits November 5, 2019 13:03

Remove object oriented programming within validation

251d239

Remove object oriented programming from prediction

4d0a3dd

Move routes to folder

2c7f764

Black code formating

8b6ecc1

- Rename File to temporary file

DigitalPhilosopher force-pushed the predict-app branch from 735a9aa to 8b6ecc1 Compare November 5, 2019 13:01

gsganden reviewed Nov 25, 2019

View reviewed changes

gsganden suggested changes Nov 25, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Predict app #121

Predict app #121

DigitalPhilosopher commented Oct 16, 2019

gsganden commented Oct 16, 2019

DigitalPhilosopher commented Oct 22, 2019

gsganden commented Oct 22, 2019

gsganden left a comment

gsganden Nov 3, 2019

DigitalPhilosopher Nov 5, 2019

gsganden Nov 25, 2019

gsganden Nov 3, 2019

DigitalPhilosopher Nov 5, 2019

gsganden Nov 3, 2019

DigitalPhilosopher Nov 5, 2019

gsganden Nov 3, 2019

DigitalPhilosopher Nov 5, 2019

DigitalPhilosopher commented Nov 5, 2019

gsganden Nov 25, 2019

gsganden left a comment

gsganden left a comment

		from ..validation.predict import validate_predict_request


		predict_route = Blueprint("predict", __name__)

Predict app #121

Are you sure you want to change the base?

Predict app #121

Conversation

DigitalPhilosopher commented Oct 16, 2019

gsganden commented Oct 16, 2019

DigitalPhilosopher commented Oct 22, 2019

gsganden commented Oct 22, 2019

gsganden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DigitalPhilosopher commented Nov 5, 2019

Choose a reason for hiding this comment

gsganden left a comment

Choose a reason for hiding this comment

gsganden left a comment

Choose a reason for hiding this comment