-
Notifications
You must be signed in to change notification settings - Fork 45.4k
make /official a Python module #3557
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
Conversation
|
Fundamentally I think the fact that this is a branch off of logging utils rather than master is going to cause awful merge conflict issues down the line. Basically you have a "delete utils" commit after the canonical util creation commit. I put together some git to forcibly update the branch to a single commit in a branch from master. I'm sure that that a proper git wizard could do better (if a git wizard wants to chime in, feel free), but I think this will suffice for not confusing git down the line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks-- a few minor changes from my end, though it looks like py3 tests are failing whereas py2 is not? Any clues what is happening there?
official/README.md
Outdated
|
|
||
| Run the models. | ||
|
|
||
| The *Official Models* is made as a Python module. To run the models directly, you must change your work directory to the top-level ***/models*** folder (the parent directory of ***/official***), and set the Python path by issuing the following command: ``` export PYTHONPATH="$PYTHONPATH:$(pwd)" ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's true that this will work, I think it's a little atypical to request that people actually cd in first. I think it might be preferable to instruct people that they have to add the top-level models folder to their python path, and illustrate like so:
export PYTHONPATH="$PYTHONPATH:/path/to/models"
Also, nit: in Github markdown, you should be able to use one backtick ( ` ) instead of three ( ``` ) for in-line code snippets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments, Karmel!
For py3 test, I have submitted a CL to set up the python path, like what we did for py2. It should pass py3 tests once the CL is approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robieta Thank you for the detailed comments! Karmel also mentioned the same issue to me. Will update the branch as a single commit.
|
What I feel confused is that when I look at all file changed, it only shows the file from commit be8c1ba. Did u change/modify to some branch that overwrites your previous change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor text changes, but other than that, LGTM.
| Run the models. | ||
|
|
||
| The *Official Models* is made as a Python module. To run the models directly, you need to add the top-level ***/models*** folder to the python path by command: ` export PYTHONPATH="$PYTHONPATH:/path/to/models" ` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more nits that I started to type out, then realized it was easier just to show the end result:
## Running the models
The *Official Models* are made available as a Python module. To run the models and associated scripts, add the top-level ***/models*** folder to the Python path with the command: `export PYTHONPATH="$PYTHONPATH:/path/to/models"`
To make Official Models easier to use, we are planning to create a pip installable Official Models package. This is being tracked in [#917](https://github.com/tensorflow/models/issues/917).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems quite reasonable.
|
运行mnist.py 出错,报The first layer in a Sequential model must get an |
make /official as a Python module to unify import of utils