Submit job with upload user's package#128
Conversation
go/paddlecloud/simplefile.go
Outdated
| "github.com/google/subcommands" | ||
| ) | ||
|
|
||
| // SimpleFileCmd define the subcommand of simple file operations |
There was a problem hiding this comment.
The comment should be a sentence, which ends with ".".
go/paddlecloud/restclient.go
Outdated
| return []byte{}, err | ||
| } | ||
|
|
||
| // open file handle |
There was a problem hiding this comment.
Comment maybe not useful.
go/paddlecloud/restclient.go
Outdated
| @@ -124,7 +124,7 @@ func PostFile(targetURL string, filename string) ([]byte, error) { | |||
| contentType := bodyWriter.FormDataContentType() | |||
| bodyWriter.Close() | |||
There was a problem hiding this comment.
Do we need to check the return of Close().
| if err = json.Unmarshal(respStr, &respObj); err != nil { | ||
| return err | ||
| } | ||
| // FIXME: Print an error if error message is not empty. Use response code instead |
There was a problem hiding this comment.
Where is response code? Is it a TODO?
There was a problem hiding this comment.
This is a enhancement to uniform the response json format, see this issue: #129
| // FIXME: Print an error if error message is not empty. Use response code instead | ||
| errMsg := respObj.(map[string]interface{})["msg"].(string) | ||
| if len(errMsg) > 0 { | ||
| fmt.Fprintf(os.Stderr, "upload file error: %s\n", errMsg) |
There was a problem hiding this comment.
return error will break the filepath.Walk call. Will fix this after the above issue is done.
| if err != nil { | ||
| return err | ||
| } | ||
| // 2. call paddlecloud server to create kubernetes job |
There was a problem hiding this comment.
This comment is not useful.
| if err != nil { | ||
| return err | ||
| } | ||
| glog.V(10).Infof("Submitting job: %s to %s\n", jsonString, config.ActiveConfig.Endpoint+"/api/v1/jobs") |
There was a problem hiding this comment.
Do we need to specify /api/v1/jobs to be constant?
There was a problem hiding this comment.
REST APIs are currently all v1, change this when we have v2
paddlecloud/paddlejob/views.py
Outdated
|
|
||
| def post(self, request, format=None): | ||
| """ | ||
| Simple up file |
There was a problem hiding this comment.
Changed to get and put
| data = file_obj.read(4096) | ||
| if not data: | ||
| break | ||
| fn.write(data) |
There was a problem hiding this comment.
Check errors may occur.
There was a problem hiding this comment.
Django will catch exceptions and return a default response.
paddlecloud/paddlejob/views.py
Outdated
|
|
||
| def get(self, request, format=None): | ||
| """ | ||
| Simple down file |
There was a problem hiding this comment.
Changed to get and put
paddlecloud/paddlejob/views.py
Outdated
|
|
||
| def __validate_path(self, request, file_path): | ||
| """ | ||
| returns error_msg. error_msg will be empty if there's no error |
There was a problem hiding this comment.
A sentence should end with '.'.
go/paddlecloud/submit.go
Outdated
| return nil | ||
| } | ||
| glog.V(10).Infof("Uploading %s...\n", filePath) | ||
| dest := "/" + path.Join("pfs", config.ActiveConfig.Name, "home", config.ActiveConfig.Username, filePath) |
There was a problem hiding this comment.
dest := "/" + path.Join("pfs", config.ActiveConfig.Name, ...
==>
dest := path.Join("/pfs", config.ActiveConfig.Name, ...
paddlecloud/paddlejob/views.py
Outdated
| gpu_count = obj.get("gpu", 0) | ||
| # jobPackage validation: startwith /pfs | ||
| # NOTE: always overwrite the job package when the directory exists | ||
| job_package =obj.get("jobPackage", "") |
There was a problem hiding this comment.
With this logic, users will use the parameter jobPackage as /pfs/xxx/home/yyy or /xxxx or a relative path, there is so much format for the jobPackage, it looks so much confusing, maybe we can only support one format, such as "/pfs/$dc/home/$username", I think it's more clearly.
There was a problem hiding this comment.
After a brief discussion, we decide to use "jobname" as default path to store the package content. So that users can find history package and program file folders if they want to find out something.
| logging.error("error submitting trainer job: %s" % e) | ||
| return utils.simple_response(500, str(e)) | ||
| return utils.simple_response(200, "OK") | ||
| return utils.simple_response(200, "") |
There was a problem hiding this comment.
Maybe this change is not necessary.
There was a problem hiding this comment.
I've changed paddlecloud client to check if msg is not empty, means there is some error.
There was a problem hiding this comment.
OK, I got it and make a mark for using HTTP status instead of check msg.
3 participants
Fix #127