-
Notifications
You must be signed in to change notification settings - Fork 705
Add doc string for models in sqlflow_models #1717
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
Add doc string for models in sqlflow_models #1717
Conversation
|
|
||
| // ExtractDocString extracts parameter documents from python doc strings | ||
| func ExtractDocString(module ...string) { | ||
| cmd := exec.Command("python", "-uc", fmt.Sprintf("__import__('extract_docstring').print_param_doc('%s')", strings.Join(module, "', '"))) |
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.
Should we make a local call here to print the docstring, other than start a process?
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.
That's somewhat hard to implement because we have to import the classes to get the docstrings. I have investigated several python bindings of golang but they are either too primitive (go-python) or not as reliable.
| if e != nil { | ||
| log.Println("ExtractDocString failed: ", e, string(output)) | ||
| } | ||
| if e := json.Unmarshal(output, &PremadeModelParamsDocs); e != nil { |
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.
Maybe add a comment that unmarshal can append entries rather than overwrite it: https://golang.org/pkg/encoding/json/#Unmarshal
| export PYTHONPATH=$GOPATH/src/sqlflow.org/sqlflow/python | ||
|
|
||
| sqlflowserver & | ||
| sleep 10 |
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.
Is it possible to change this sleep to a while loop that pings the SQLFlow server?
Partially fix #1558
A pitfall is that it takes longer for repl/sqlflowserver to start-up because we have to
import tensorflow