Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-558. Define Swagger API for pre-defined template submission #382

Closed
wants to merge 15 commits into from

Conversation

JohnTing
Copy link
Contributor

@JohnTing JohnTing commented Aug 19, 2020

What is this PR for?

Make basic rest api for submi template.

Convert submitted template to experiment
post
http://localhost/V1/template/submit

What type of PR is it?

[Bug Fix | Improvement | Feature | Documentation | Hot Fix | Refactoring]

Todos

  • - API
  • - test

What is the Jira issue?

https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-558

How should this be tested?

# Register experiment template
curl -X POST -H "Content-Type: application/json" -d '
{
  "name": "tf-mnist-test",
  "author": "author",
  "description": "This is a template to run tf-mnist\n",
  "parameters": [
    {
      "name": "training.learning_rate",
      "value": 0.1,
      "required": true,
      "description": " mnist learning_rate "
    },
    {
      "name": "training.batch_size",
      "value": 150,
      "required": false,
      "description": "This is batch size of training"
    }
  ],
  "experimentSpec": {
    "meta": {
      "cmd": "python /var/tf_mnist/mnist_with_summaries.py --log_dir=/train/log --learning_rate={{training.learning_rate}} --batch_size={{training.batch_size}}",
      "name": "tf-mnist-template-test",
      "envVars": {
        "ENV1": "ENV1"
      },
      "framework": "TensorFlow",
      "namespace": "default"
    },
    "spec": {
      "Ps": {
        "replicas": 1,
        "resources": "cpu=1,memory=1024M"
      },
      "Worker": {
        "replicas": 1,
        "resources": "cpu=1,memory=1024M"
      }
    },
    "environment": {
      "image": "gcr.io/kubeflow-ci/tf-mnist-with-summaries:1.0"
    }
  }
}
' http://127.0.0.1:8080/api/v1/template
# Submit experiment template to experiment 
curl -X POST -H "Content-Type: application/json" -d '
{
    "name": "tf-mnist-test", 
    "params": {
        "training.learning_rate":"0.01", 
        "training.batch_size":"150"
    }
}
' http://127.0.0.1:8080/api/v1/template/submit

If the submission is successful, it will return

{
    "status": "OK",
    "code": 200,
    "success": true,
    "message": null,
    "result": {
        "experimentId": "experiment_1597853926000_0035",
        "name": "tf-mnist-template-test",
        "uid": "0a79d641-871e-4ba6-9e3e-eab1d4690f4e",
        "status": "Accepted",
        "acceptedTime": "2020-08-22T18:36:32.000+08:00",
        "createdTime": null,
        "runningTime": null,
        "finishedTime": null,
        "spec": {
            "meta": {
                "name": "tf-mnist-template-test",
                "namespace": "default",
                "framework": "TensorFlow",
                "cmd": "python /var/tf_mnist/mnist_with_summaries.py --log_dir=/train/log --learning_rate=0.01 --batch_size=150",
                "envVars": {
                    "ENV1": "ENV1"
                }
            },
            "environment": {
                "name": null,
                "dockerImage": null,
                "kernelSpec": null,
                "description": null,
                "image": "gcr.io/kubeflow-ci/tf-mnist-with-summaries:1.0"
            },
            "spec": {
                "Ps": {
                    "replicas": 1,
                    "resources": "cpu=1,memory=1024M",
                    "name": null,
                    "image": null,
                    "cmd": null,
                    "envVars": null,
                    "resourceMap": {
                        "memory": "1024M",
                        "cpu": "1"
                    }
                },
                "Worker": {
                    "replicas": 1,
                    "resources": "cpu=1,memory=1024M",
                    "name": null,
                    "image": null,
                    "cmd": null,
                    "envVars": null,
                    "resourceMap": {
                        "memory": "1024M",
                        "cpu": "1"
                    }
                }
            }
        }
    },
    "attributes": {}
}

Screenshots (if appropriate)

image

Questions:

  • Does the licenses files need update? Yes/No
  • Is there breaking changes for older versions? Yes/No
  • Does this needs documentation? Yes/No

@JohnTing JohnTing changed the title SUBMARINE-558.Define Swagger API for pre-defined template submission SUBMARINE-558. Define Swagger API for pre-defined template submission Aug 19, 2020
@JohnTing JohnTing marked this pull request as ready for review August 21, 2020 07:39
@jiwq jiwq self-requested a review August 22, 2020 14:20
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JohnTing for working on this.
Some suggestion in the below.

  1. Could we remove test_template_2.json? it's same as test_template_1.json
  2. Need to change directory name submarine-server/server-core/src/test/resources/experimenttemplate/test_template_1.json to submarine-server/server-core/src/test/resources/experimentTemplate/test_template_1.json

JohnTing and others added 8 commits August 24, 2020 21:48
…e/server/experimenttemplate/ExperimentTemplateManager.java

Co-authored-by: HUAN-PING SU <pingsutw@gmail.com>
…e/server/experimenttemplate/ExperimentTemplateManager.java

Co-authored-by: HUAN-PING SU <pingsutw@gmail.com>
…e/server/experimenttemplate/ExperimentTemplateManager.java

Co-authored-by: HUAN-PING SU <pingsutw@gmail.com>
…t/ExperimentTemplateManagerRestApiIT.java

Co-authored-by: HUAN-PING SU <pingsutw@gmail.com>
@wangdatan
Copy link
Contributor

Thanks @JohnTing for the patch, I reviewed registration flow, and what does the API looks like. I actually like the new implementation more than the original design of https://github.com/apache/submarine/blob/master/docs/design/experiment-implementation.md. The parameterized implementation is very clean and very easy for users to register and understand. Great job!

I will leave the detailed code review works to others, at a high-level, the implementation looks good!

Copy link
Member

@jiwq jiwq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnTing Thanks for your contribution. But I have a doubt "how can the user submit the experiment by template?" I saw the REST api /api/v1/template/submit, but I can't get its meaning. Such as 'create new template' or others? In my opinion, if we provide the template resource, the experiment resource depends on template. So we should extend the experiment api to support accepts the template id and related params to training.

@JohnTing
Copy link
Contributor Author

JohnTing commented Aug 29, 2020

@JohnTing Thanks for your contribution. But I have a doubt "how can the user submit the experiment by template?" I saw the REST api /api/v1/template/submit, but I can't get its meaning. Such as 'create new template' or others? In my opinion, if we provide the template resource, the experiment resource depends on template. So we should extend the experiment api to support accepts the template id and related params to training.

@jiwq, Thanks for your review

My last PR (SUBMARINE-559)
[API] Define Swagger API for pre-defined template registration/delete, etc.
Support methods such as create new template or delete template

get template list

/api/v1/template

get template

get /api/v1/template/{templateName}

post template

post /api/v1/template

patch template

patch /api/v1/template/{templateName}

delete template

delete /api/v1/template/{templateName}

then this PR uses the registered template to submit as an experiment

submit template as an experiment

post /api/v1/template/submit

The format of the registered template includes a complete experiment, but the template maker can change some parameters into variables for template users to adjust.

@jiwq
Copy link
Member

jiwq commented Aug 29, 2020

@JohnTing Thanks for your contribution. But I have a doubt "how can the user submit the experiment by template?" I saw the REST api /api/v1/template/submit, but I can't get its meaning. Such as 'create new template' or others? In my opinion, if we provide the template resource, the experiment resource depends on template. So we should extend the experiment api to support accepts the template id and related params to training.

@jiwq, Thanks for your review

My last PR (SUBMARINE-559)
[API] Define Swagger API for pre-defined template registration/delete, etc.
Support methods such as create new template or delete template

get template list

/api/v1/template

get template

get /api/v1/template/{templateName}

post template

post /api/v1/template

patch template

patch /api/v1/template/{templateName}

delete template

delete /api/v1/template/{templateName}

then this PR uses the registered template to submit as an experiment

submit template as an experiment

post /api/v1/template/submit

The format of the registered template includes a complete experiment, but the template maker can change some parameters into variables for template users to adjust.

Got. I think POST /api/v1/template/submit is not a good design, extend the POST /api/v1/experiment is the better selected (more info see mine above comment). Any thoughts?

@JohnTing
Copy link
Contributor Author

JohnTing commented Aug 29, 2020

@jiwq
If I only need to change the code place and path, there is no problem.
Do I need to extend the API as POST /api/v1/experiment/submit or POST /api/v1/experiment/?

@jiwq
Copy link
Member

jiwq commented Sep 5, 2020

@jiwq
If I only need to change the code place and path, there is no problem.
Do I need to extend the API as POST /api/v1/experiment/submit or POST /api/v1/experiment/?

I recommend use /api/v1/experiment as the resource and extend the associated operations.

Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnTing , What is the status of this PR now? Has this PR been completed?

@JohnTing
Copy link
Contributor Author

@JohnTing , What is the status of this PR now? Has this PR been completed?

Thanks for @xunliu comment, this pr has been completed.

Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnTing Thank you contibution this feature.
Will merge if no more comments.

@asfgit asfgit closed this in 57eb259 Sep 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants