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

Fix API docs #10268

Merged
merged 10 commits into from
Nov 19, 2021
Merged

Fix API docs #10268

merged 10 commits into from
Nov 19, 2021

Conversation

tayfun
Copy link
Contributor

@tayfun tayfun commented Nov 17, 2021

Proposed changes:

Fixes #10148

  • Updates /model/train endpoint in docs to only accept YAML input
  • Updates /model endpoint to clarify model path

@tayfun tayfun requested review from ancalita and indam23 November 17, 2021 16:26
@tayfun tayfun requested a review from a team as a code owner November 17, 2021 16:26
Copy link
Contributor

@indam23 indam23 left a comment

Choose a reason for hiding this comment

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

Mostly concerned about the function removal - not sure that should be happening here?

docs/static/spec/rasa.yml Outdated Show resolved Hide resolved
rasa/server.py Show resolved Hide resolved
$ref: '#/components/schemas/DomainFile'
config:
$ref: '#/components/schemas/ConfigFile'
intents:
Copy link
Contributor

Choose a reason for hiding this comment

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

This section should include all properties (missing ones from the domain include actions, entities, e2e_actions, config) and be ordered (IMO) by config.yml, then domain.yml, then the data files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section should include all properties (missing ones from the domain include actions, entities, e2e_actions, config) and be ordered (IMO) by config.yml, then domain.yml, then the data files

@melindaloubser1 are you sure actions etc. would be handled here? I don't see it in stale docs either: https://rasa.com/docs/rasa/pages/http-api#operation/trainModel I've taken the example and schema from unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't all of this covered by #/components/schemas/DomainFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joejuzl It's not under domain anymore, request body is flattened. This is why I've newly added this schema. Also, examples inside schemas do not work for any content type except for JSON in redocly, so example keys were removed and a separate example on the same level as schema was added. The other thing is I've simplified the example, previously there were 6 intents in the example request, I've added 2 to show how a list can be sent, I think people can extrapolate that and add more intents if needed.

Comment on lines 1885 to 1892
YAMLTrainingRequest: |
intents:
- greet
- goodbye

responses:
utter_greet:
- text: "Hey! How are you?"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same on this section: reorder by config.yml then domain.yml then data, and add all keys even if they're empty e.g. actions: []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same on this section: reorder by config.yml then domain.yml then data, and add all keys even if they're empty e.g. actions: []

@melindaloubser1 I don't think actions are used here. I don't understand the ordering, why would it session_config come before others? Or do you mean pipeline comes first, then policies?

Copy link
Contributor

Choose a reason for hiding this comment

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

For this particular example, there are no custom actions to pass, so you are correct. However, in a bot with custom actions, or slots, or entities, these sections would need to be included.

Re. the ordering: Sorry for the shorthand, I meant the contents of config.yml, so yes, pipeline + policies, then domain content (intents, entities, actions, e2e_actions, forms, slots, responses, config<- this is a key in the domain in 3.0, which is quite confusing) then content of nlu, rules and stories. It's not required to do it this way for the example to work, but will make it much easier for readers to understand the example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@melindaloubser1 Yeah I meant do you know custom actions are used if provided in request body like here? I don't see that in code, and I don't see them being tested in unit tests. I wouldn't be surprised if not all CLI options were supported here.

Copy link
Contributor

Choose a reason for hiding this comment

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

They would have to be supported to train on stories which include custom actions I believe

@tayfun tayfun requested review from wochinge, joejuzl and indam23 and removed request for ancalita November 17, 2021 17:59
$ref: '#/components/schemas/DomainFile'
config:
$ref: '#/components/schemas/ConfigFile'
intents:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't all of this covered by #/components/schemas/DomainFile?

type: integer
carry_over_slots_to_new_session:
type: boolean
pipeline:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change from #/components/schemas/ConfigFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joejuzl Similar to domain, request body is flattened, config key doesn't exist anymore.

Copy link
Contributor

@joejuzl joejuzl Nov 18, 2021

Choose a reason for hiding this comment

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

is there no way to include the contents of #/components/schemas/ConfigFile at the top level, i.e. without it being under its own key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried using allOf but couldn't get it to work. To test this, I've run make livedocs in top level and then added:

    TrainingSchema:
      allOf:
        - $ref: '#/components/schemas/DomainFile'
        - $ref: '#/components/schemas/ConfigFile'

to the bottom of the file and referenced it in requestBody as:

          application/yaml:
            schema:
              - $ref: '#/components/schemas/TrainingSchema'

I don't see schema and only see any in the docs output in that case. @joejuzl @melindaloubser1 do you know a better way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that second part's schema should not be a list:

         application/yaml:
            schema:
              $ref: '#/components/schemas/TrainingSchema'

Notice the dash is missing. This is still not showing the schema, but instead of any, I get string (TrainingSchema)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I haven't found another way either - Only experimented briefly but ran into the same issues you did @tayfun

@tayfun tayfun requested a review from joejuzl November 18, 2021 18:39
Copy link
Contributor

@joejuzl joejuzl left a comment

Choose a reason for hiding this comment

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

Bit annoying we can't inline the other schemas... but at least it's correct now! 😁

@tayfun tayfun enabled auto-merge (squash) November 19, 2021 11:45
@tayfun tayfun merged commit c2b5176 into main Nov 19, 2021
@tayfun tayfun deleted the tayfun/10148-fix-api-docs branch November 19, 2021 14:51
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

Successfully merging this pull request may close these issues.

Model API QA: Errors met when testing model training and replacing loaded model
3 participants