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

Fixed schema refs for ml.yaml #489

Merged
merged 7 commits into from
Aug 12, 2024

Conversation

nhtruong
Copy link
Collaborator

The body schemas for some bodies in the ml namespace were incorrectly referenced to the content-type instead of their respective schema property.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@nhtruong nhtruong added the skip-changelog No need to update CHANGELOG. label Aug 11, 2024
Copy link
Contributor

github-actions bot commented Aug 11, 2024

Changes Analysis

Commit SHA: 3d82522
Comparing To SHA: d7e6971

API Changes

Summary

├─┬Paths
│ ├─┬/_plugins/_ml/model_groups/_register
│ │ └─┬POST
│ │   └─┬Responses
│ │     └─┬200
│ │       └─┬application/json
│ │         └──[➕] schema (45064:7)❌ 
│ ├─┬/_plugins/_ml/model_groups/{model_group_id}
│ │ ├─┬GET
│ │ │ └─┬Responses
│ │ │   └─┬200
│ │ │     └─┬application/json
│ │ │       └──[➕] schema (45038:7)❌ 
│ │ └─┬DELETE
│ │   └─┬Responses
│ │     └─┬200
│ │       └─┬application/json
│ │         └──[➕] schema (28463:7)❌ 
│ ├─┬/_plugins/_ml/models/_search
│ │ └─┬GET
│ │   ├─┬Responses
│ │   │ └─┬200
│ │   │   └─┬application/json
│ │   │     └──[➕] schema (45115:7)❌ 
│ │   └─┬Requestbody
│ │     └─┬application/json
│ │       └──[➕] schema (45103:7)❌ 
│ ├─┬/_plugins/_ml/tasks/{task_id}
│ │ └─┬GET
│ │   └─┬Responses
│ │     └─┬200
│ │       └─┬application/json
│ │         └──[➕] schema (45122:7)❌ 
│ ├─┬/_plugins/_ml/models/_register
│ │ └─┬POST
│ │   └─┬Responses
│ │     └─┬200
│ │       └─┬application/json
│ │         └──[➕] schema (25183:13)❌ 
│ └─┬/_plugins/_ml/models/{model_id}
│   └─┬DELETE
│     └─┬Responses
│       └─┬200
│         └─┬application/json
│           └──[➕] schema (28463:7)❌ 
└─┬Components
  └─┬ml._common:Task
    ├──[➕] properties (45126:9)
    ├─┬state
    │ └──[➖] description (45113:24)
    ├─┬model_id
    │ └──[➖] description (45110:24)
    └─┬task_type
      └──[➕] enum (45142:15)

Document Element Total Changes Breaking Changes
paths 8 8
components 4 0
  • BREAKING Changes: 8 out of 12
  • Removals: 2
  • Additions: 10
  • Breaking Additions: 8

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10358041600/artifacts/1803416154

API Coverage

Before After Δ
Covered (%) 510 (49.95 %) 510 (49.95 %) 0 (0 %)
Uncovered (%) 511 (50.05 %) 511 (50.05 %) 0 (0 %)
Unknown 24 24 0

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Should the checker have caught this as additional fields or something?

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks like ML tests started failing.

@nhtruong
Copy link
Collaborator Author

Should the checker have caught this as additional fields or something?

We should create JSON-schemas for the namespace and category files to catch this among others, especially on catching typos and enforcing rules for extensions.

@dblock
Copy link
Member

dblock commented Aug 12, 2024

Should the checker have caught this as additional fields or something?

We should create JSON-schemas for the namespace and category files to catch this among others, especially on catching typos and enforcing rules for extensions.

Yes, you want to take it? #492

The tests here are failing though, so you'll need to add a fix.

@nhtruong
Copy link
Collaborator Author

nhtruong commented Aug 12, 2024

The tests here are failing though, so you'll need to add a fix.

I saw that. I'm on it.
It also reveals a potential bug in the test framework: When the response schema is not defined in the spec (At least the framework thought so in this case because the schema was referenced in the wrong place), it skips response payload test and still passes the chapter. I think it should throw an error (spec not found) in this case just like we will fail the test if a property is missing (In this case all fields are missing 😆 yet the test passed)

@dblock
Copy link
Member

dblock commented Aug 12, 2024

The tests here are failing though, so you'll need to add a fix.

I saw that. I'm on it. It also reveals a potential bug in the test framework: When the response schema is not defined in the spec (At least the framework thought so in this case because the schema was referenced in the wrong place), it skips response payload test and still passes the chapter. I think it should throw an error (spec not found) in this case just like we will fail the test if a property is missing (In this case all fields are missing 😆 yet the test passed)

Feel free to open this one separately and I can take care of it.

#
Signed-off-by: Theo Truong <[email protected]>
@nhtruong nhtruong marked this pull request as draft August 12, 2024 18:55
#
Signed-off-by: Theo Truong <[email protected]>
#
Signed-off-by: Theo Truong <[email protected]>
@nhtruong
Copy link
Collaborator Author

nhtruong commented Aug 12, 2024

@dblock I think I've found another bug:

Skipped because version 2.17.0 does not satisfy >= 2.11, < 3.0.

#500

#
Signed-off-by: Theo Truong <[email protected]>
Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
548 206 (37.59 %)

#
Signed-off-by: Theo Truong <[email protected]>
#
Signed-off-by: Theo Truong <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

See below.

ml.register_model@200:
content:
application/json:
$ref: '../schemas/ml._common.yaml#/components/schemas/Task'
schema:
Copy link
Member

Choose a reason for hiding this comment

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

Is this not a task? In tests we register a model then wait it the task to complete.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The response is not the entire Task object, which is different from get_task's response. This only returns the task_id for you to check on it later.

@dblock dblock merged commit 42fd4d7 into opensearch-project:main Aug 12, 2024
16 checks passed
@nhtruong nhtruong deleted the fix_SearchModelsQuery branch August 13, 2024 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No need to update CHANGELOG.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants