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

Had to update LOCAL_MODEL_DIR location for chicago_taxi_pipeline example #9

Closed
aaronlelevier opened this issue Mar 9, 2019 · 7 comments

Comments

@aaronlelevier
Copy link
Contributor

I was following the tfx/examples/chicago_taxi_pipeline/README.md. Everything worked pretty flawlessly. I did have to make a change to:

https://github.com/tensorflow/tfx/blob/master/examples/chicago_taxi/start_model_server_local.sh#L31

I changed LOCAL_MODEL_DIR to:

LOCAL_MODEL_DIR=$TAXI_DIR/serving_model/taxi_simple/

It looks like the code in the chicago_taxi dir is different from the chicago_taxi_pipeline dir example for where the serving model is stored.

Should I submit a small PR for the change?

If so, would it be preferred to have a copy of the start_model_server_local.sh file in both chicago_taxi and chicago_taxi with different values for LOCAL_MODEL_DIR or it's better to have one start_model_server_local.sh file that accepts a LOCAL_MODEL_DIR argument?

If I do a PR, I can also include an update to the README.md to document the change.

my software version

Mac OSX
Python 2.7

p.s.

I just want to thank you for all of the work that Google has done on TFX. It's amazing for ML to be so accessible and for the ecosystem to come together like this.

Thanks

@krazyhaas
Copy link

Hi Aaron. Thanks for the feedback and glad to hear your experience was almost perfect. A PR would be much appreciated. If you do, my preference would be to have start_model_server_local.sh accept the LOCAL_MODEL_DIR argument instead of copying the script.

-K

@1025KB
Copy link
Collaborator

1025KB commented Mar 11, 2019

at the end of README.md, it has instructions about how to modify the start_model_server_local.sh to fit chicago_taxi_pipeline

@aaronlelevier
Copy link
Contributor Author

So remove the additional start_model_server_local.sh file, leave the readme documentation, and just fixup the path where the model is being saved?

@1025KB
Copy link
Collaborator

1025KB commented Mar 11, 2019

the serving and client examples in chicago_taxi dir works for chicago_taxi_pipeline dir excepts some path changes listed in README.md (the current path setup is for chicago_taxi example instead of chicago_taxi_pipeline example)

@aaronlelevier
Copy link
Contributor Author

Ok, I'm going to update the PR. Thanks

@aaronlelevier
Copy link
Contributor Author

Okay, I updated the PR #10 so the only changes are updating the path where the model is saved in the readme documentation. The programmer can change the path for LOCAL_MODEL_DIR in start_model_server_local.sh as it was documented before.

@1025KB
Copy link
Collaborator

1025KB commented Mar 12, 2019

Thanks Aaron!

@1025KB 1025KB closed this as completed Mar 12, 2019
ruoyu90 pushed a commit to ruoyu90/tfx that referenced this issue Aug 28, 2019
Remove redundant ID header from RFC template
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants