Python fix + cleanup by wagoodman · Pull Request #123 · metaparticle-io/package

wagoodman · 2018-04-19T23:54:22Z

This isn't associated with any particular issue, and I didn't think it would be helpful creating an issue + PR for each of these fixes, so I made a general cleanup PR. Let me know if this is bothersome and I can break it up. Here is the summary of changes:

There is a json syntax error in the version.json and the json package was misused, both causing a critical failure on startup.
Looks like unittest won out over tox, so I removed tox references and updated the Makefile with the right test cmds
Updated the examples to be generally runnable + show verbose output. With simple.py only one entrypoint was chosen given that there is a web.py there to illustrate how to use metaparticle with port requirements.
The logger names for the runner and builder have been changed to the modules parent name to be more generic. E.g. instead of metaparticle_pkg.runner.docker_runner it is just metaparticle_pkg.runner since only one runner can be used at a time, and changing runners shouldn't involve a code change to see the correct logs.
I wasn't able to reproduce https://github.com/docker/docker-py/issues/300 exactly as the docker_runner suggested, however, I believe it was a misdiagnosis. Without a tty in the container the buffered output that python defaults to will not be shown (so nothing was visible from a docker logs -f ...). The easy fix is to use unbuffered python (via the -u switch on the entrypoint in the Dockerfile).

brendandburns · 2018-04-20T04:22:52Z

THis looks great, thanks! I'll merge it into my local dev and test, and then merge to master assuming things look good.

Thanks Again.

tomkukral · 2018-04-20T06:07:33Z

Good job! Thanks for fixing my mistakes. 👍

huitseeker

Commented on a few nits, this looks great!

huitseeker · 2018-05-01T16:27:59Z

python/metaparticle_pkg/builder/docker_builder.py

 from docker import APIClient

-logger = logging.getLogger(__name__)
+logger = logging.getLogger('.'.join(__name__.split('.')[:-1]))


OK, that gets us common logging for builder and runner. But this is a specific choice, so it would be nice to have a comment line indicating the purpose (here and in the sibling line of this change, in runner).

Makes sense, I'll make the adjustment

huitseeker · 2018-05-01T16:31:32Z

python/setup.cfg

 max-line-length = 160
+exclude = 
+    ./.git,
+    ./venv


nit: venv as a subdirectory is very specific/subjective. This might surprise some.

Understood, however, this is also referenced in the Makefile https://github.com/metaparticle-io/package/blob/master/python/Makefile

huitseeker · 2018-05-01T16:33:11Z

python/setup.py

 import json

-config = json.loads('./metaparticle_pkg/version.json')
+config = json.load(open('./metaparticle_pkg/version.json', 'r'))


with open('...') as ...: for idiomacy?

huitseeker · 2018-05-01T16:35:35Z

python/examples/simple/requirements.txt

@@ -1,2 +1,2 @@
 six==1.10.0
-metaparticle_pkg==0.4.1
+metaparticle_pkg==0.4.1


Not a big fan of explicit package #s in master, but my opinions are better expressed in #126 – YMMV

Actually, this would easily get around a few of the problems i was investigating... I'll make the change. (One outstanding problem with this approach: this wont make it any easier to dev on this package and test it within a container though)

brendandburns · 2018-05-11T03:49:31Z

Thanks!

* Updated Java package tutorial instructions (metaparticle-io#110) * Correct the expected value of the @Package/repository value * Mention the need for the @Package/publish=true field * Removed use of the @Runtime/publicAddress field since the tutorial does not appear to be ACI specific * Correct the type of service created on the remote Kubernetes cluster * Added missing unit test dependencies (metaparticle-io#109) * Update README.md (metaparticle-io#106) Slight typo. "a nd language of choice" ===> "and language of choice" * Create dotnet testrunner (metaparticle-io#87) * fix up examples and documentation * fix up indenting * add initial code for a dotnet test runner as part of metaparticle * add missed file * renamed env vars to be conssitent and also moved braces to new lines * Add travis for dotnet (#3) * add initial travis file * fix up missing dash * try again going into the right directory this time * no need for mono and we can set the dotnet core version * make build.sh executable * see what directory we are on * set to unix type * build the examples * move tests to attributes rather than using env var * Initial Set of Unit Tests For DotNet Metaparticle.Package (metaparticle-io#111) * Create unit test project and added tests for Metaparticle.Package.Config * Added unit tests for Config and an initial set of tests for the Driver * Minor update to Java sharding tutorial instructions (metaparticle-io#112) (metaparticle-io#113) * Fix typo in tutorial (metaparticle-io#114) * Support containerizing of Spring Boot apps (metaparticle-io#115) * Necessary to upgrade PowerMock version in order to run tests * Additions to gitignore file * Support sharding. (metaparticle-io#118) * Update definition of local functions with explicit const (metaparticle-io#120) * Add some words on Spring Boot support to package Java tutorial (metaparticle-io#121) * Follow up to previous PR for issue metaparticle-io#6 * Lazy intialize the Docker client. (metaparticle-io#119) * Fix missing comma, go fmt (metaparticle-io#122) * Adding rust (metaparticle-io#86) * learned a touch of rust * no decorator * base rust entrypoint * with bare docker builder and executor (#3) * Real traits jim (#4) * add placeholder functions for actual interface * move builder trait into builder module * move docker builder struct into correct module * move existing functions onto trait implementation * move executor trait to appropriate module * move executor struct into correct module * initial builder working * added docker run support; cleaned up command execs * refactor to str refs; added readme * We need strings here, I think The result of the format! call is a string, (as is the result of other ways of getting a u64 into a string). This change makes the method compile without changing anything's type signature. There might be other was to accomplish this, but I haven't found one. * added web example * use executable's path as Docker context * wrapped docker cmds in sh * write dockerfile to docker context explicitly * fix copy paste error in README * Python fix + cleanup (metaparticle-io#123) * general cleanup * nested options should be processed into objects from dictionaries * comments on logging; pythonic json read * Drive-by cleanups (metaparticle-io#126) * Let requirements match the latest version * Make print statements Python3-compatible * insulate more against Py2 -> Py3 changes * Add python additionalFiles option (metaparticle-io#125) * add python additionalFiles option * in case a dir is given * added tests for additionalFiles * Lazy intialize the Docker client. (metaparticle-io#127) * more efficient base image (metaparticle-io#131) * more efficient base image * squashed layers * removed breaking character * adding certs * sized down to 6mb and tested with simple http server

wagoodman force-pushed the general-cleanup branch from 50bbda4 to 2da8299 Compare April 20, 2018 00:05

general cleanup

8175001

wagoodman force-pushed the general-cleanup branch from 2da8299 to 8175001 Compare April 20, 2018 00:07

nested options should be processed into objects from dictionaries

c6c3cec

huitseeker mentioned this pull request May 1, 2018

Drive-by cleanups #126

Merged

huitseeker reviewed May 1, 2018

View reviewed changes

comments on logging; pythonic json read

4f4360e

wagoodman force-pushed the general-cleanup branch from 8b9f717 to 4f4360e Compare May 5, 2018 22:06

brendandburns merged commit b716c40 into metaparticle-io:master May 11, 2018

wagoodman deleted the general-cleanup branch May 11, 2018 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Python fix + cleanup#123

Python fix + cleanup#123
brendandburns merged 3 commits intometaparticle-io:masterfrom
wagoodman:general-cleanup

wagoodman commented Apr 19, 2018 •

edited

Loading

Uh oh!

brendandburns commented Apr 20, 2018

Uh oh!

tomkukral commented Apr 20, 2018

Uh oh!

huitseeker left a comment

Uh oh!

huitseeker May 1, 2018 •

edited

Loading

Uh oh!

wagoodman May 5, 2018

Uh oh!

huitseeker May 1, 2018

Uh oh!

wagoodman May 5, 2018

Uh oh!

huitseeker May 1, 2018

Uh oh!

wagoodman May 5, 2018

Uh oh!

huitseeker May 1, 2018

Uh oh!

wagoodman May 5, 2018

Uh oh!

brendandburns commented May 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

wagoodman commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brendandburns commented Apr 20, 2018

Uh oh!

tomkukral commented Apr 20, 2018

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

huitseeker May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wagoodman May 5, 2018

Choose a reason for hiding this comment

Uh oh!

huitseeker May 1, 2018

Choose a reason for hiding this comment

Uh oh!

wagoodman May 5, 2018

Choose a reason for hiding this comment

Uh oh!

huitseeker May 1, 2018

Choose a reason for hiding this comment

Uh oh!

wagoodman May 5, 2018

Choose a reason for hiding this comment

Uh oh!

huitseeker May 1, 2018

Choose a reason for hiding this comment

Uh oh!

wagoodman May 5, 2018

Choose a reason for hiding this comment

Uh oh!

brendandburns commented May 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wagoodman commented Apr 19, 2018 •

edited

Loading

huitseeker May 1, 2018 •

edited

Loading