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

[DOCS] Add code snippet testing in more ML APIs #31339

Merged
merged 4 commits into from
Jun 21, 2018

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Jun 14, 2018

This PR adds code snippet testing for more machine learning APIs.

Related to #30665

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs

@lcawl lcawl added the WIP label Jun 14, 2018
@davidkyle
Copy link
Member

The build is failing for the preview-datafeed line 52 snippet with the error

ERROR   1.04s | XDocsClientYamlTestSuiteIT.test {yaml=en/rest-api/ml/preview-datafeed/line_52} <<< FAILURES!
   > Throwable #1: java.lang.RuntimeException: Failure at [en/rest-api/ml/preview-datafeed:75]: element was a list, but [_shards] was not numeric

The problem is that the test framework injects the assertion - is_false: _shards.failures into the yml test but the response is a JSON array rather than an object so the check for _shards.failures throws an exception because _shards can't be an index to the array.

This feels like a corner case -I haven't seen this problem before-, one solution is the change the format of the preview datafeed response to be NDJSON although technically that is a breaking change. Another option is to add a flag to the snippet expects_array that would forgo the _shards.failures check.

@nik9000 do you have any ideas please?

The generated yml test looks like this:

  - do:
      raw:
        method: GET
        path: "_xpack/ml/datafeeds/datafeed-farequote/_preview"
  - is_false: _shards.failures
  - match: 
      $body: 
        [
          {
            "time": 1454803200000,
            "airline": "JZA",
            "doc_count": 5,
            "responsetime": 990.4628295898438
          },
          {
            "time": 1454803200000,
            "airline": "JBU",
            "doc_count": 23,
            "responsetime": 877.5927124023438
          },
          {
            "time": 1454803200000,
            "airline": "KLM",
            "doc_count": 42,
            "responsetime": 1355.481201171875
          }
        ]

@nik9000
Copy link
Member

nik9000 commented Jun 18, 2018

@davidkyle, we're talking about this API, right?

That assertion is really just there because searches are so command and we don't otherwise check for failing shards. I wonder if we'd be better off just asserting that after a search.

@davidkyle
Copy link
Member

@nik9000 Yes the preview data feed API.

It would be nice to enable the check only for searches (and GET document/bulk APIs?) as it is redundant for the majority for cases especially in x-pack. The change appears a simple one I'll have a go at it if you don't mind.

I would also be happy to change the format of the preview API and make it a breaking change in 7.

@nik9000
Copy link
Member

nik9000 commented Jun 19, 2018

I would also be happy to change the format of the preview API and make it a breaking change in 7.

I don't have a problem with the format of the preview API. I don't really know it well enough to have an opinion.

The change appears a simple one I'll have a go at it if you don't mind.

Sure! A whitelist of a few APIs we should generate this for is fine with me!

@droberts195
Copy link
Contributor

I would also be happy to change the format of the preview API and make it a breaking change in 7.

Before doing this we need to consider:

  1. Are there any other APIs that return ND-JSON as a result type?
  2. Do the client maintainers have to do bespoke work to consume ND-JSON as a result type for a particular API?

If the answers are no and yes then we shouldn't make this change. If the answers are yes and yes then we need to carefully weigh up the benefit against the extra work we're piling onto the clients team.

@lcawl
Copy link
Contributor Author

lcawl commented Jun 19, 2018

(I am moving all but the preview datafeed API work out of this PR). The forecast API piece is moving to #31439

@davidkyle
Copy link
Member

@lcawl I merged the fix for the datafeed preview API (#31430) into 7, 6.4 and 6.3.1 if you rebase you should get a green build now.

@lcawl lcawl merged commit 4385915 into elastic:master Jun 21, 2018
@lcawl lcawl deleted the lcawley-ml-tests branch June 21, 2018 18:32
lcawl added a commit to lcawl/elasticsearch that referenced this pull request Jun 21, 2018
dnhatn added a commit that referenced this pull request Jun 23, 2018
* master:
  Add get field mappings to High Level REST API Client (#31423)
  [DOCS] Updates Watcher examples for code testing (#31152)
  TEST: Add bwc recovery tests with synced-flush index
  [DOCS] Move sql to docs (#31474)
  [DOCS] Move monitoring to docs folder (#31477)
  Core: Combine doExecute methods in TransportAction (#31517)
  IndexShard should not return null stats (#31528)
  fix repository update with the same settings but different type (#31458)
  Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527)
  Node selector per client rather than per request (#31471)
  Core: Combine messageRecieved methods in TransportRequestHandler (#31519)
  Upgrade to Lucene 7.4.0. (#31529)
  [ML] Add ML filter update API (#31437)
  Allow multiple unicast host providers (#31509)
  Avoid deprecation warning when running the ML datafeed extractor. (#31463)
  REST high-level client: add simulate pipeline API (#31158)
  Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507)
  [PkiRealm] Invalidate cache on role mappings change (#31510)
  [Security] Check auth scheme case insensitively (#31490)
  In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514)
  [DOCS] Fix REST tests in SQL docs
  [DOCS] Add code snippet testing in more ML APIs (#31339)
  Core: Remove ThreadPool from base TransportAction (#31492)
  [DOCS] Remove fixed file from build.gradle
  Rename createNewTranslog to fileBasedRecovery (#31508)
  Test: Skip assertion on windows
  [DOCS] Creates field and document level security overview (#30937)
  [DOCS] Significantly improve SQL docs
  [DOCS] Move migration APIs to docs (#31473)
  Core: Convert TransportAction.execute uses to client calls (#31487)
  Return transport addresses from UnicastHostsProvider (#31426)
  Ensure local addresses aren't null (#31440)
  Remove unused generic type for client execute method (#31444)
  Introduce http and tcp server channels (#31446)
dnhatn added a commit that referenced this pull request Jun 23, 2018
* 6.x:
  Avoid sending duplicate remote failed shard requests (#31313)
  Add get field mappings to High Level REST API Client Relates to #27205
  [DOCS] Updates Watcher examples for code testing (#31152)
  [DOCS] Move monitoring to docs folder (#31477)
  [DOCS] Fixes SQL docs in nav
  [DOCS] Move sql to docs
  IndexShard should not return null stats - empty stats or AlreadyCloseException if it's closed is better
  Clarify that IP range data can be specified in CIDR notation. (#31374)
  Remove some cases in FieldTypeLookupTests that are no longer relevant. (#31381)
  In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514)
  fix repository update with the same settings but different type
  Revert "AwaitsFix FullClusterRestartIT#testRecovery"
  Upgrade to Lucene 7.4.0. (#31529)
  Avoid deprecation warning when running the ML datafeed extractor. (#31463)
  Retry synced-flush in FullClusterRestartIT#testRecovery
  Allow multiple unicast host providers (#31509)
  [ML] Add ML filter update API (#31437)
  AwaitsFix FullClusterRestartIT#testRecovery
  Fix missing historyUUID in peer recovery when rolling upgrade 5.x to 6.3 (#31506)
  Remove QueryCachingPolicy#ALWAYS_CACHE (#31451)
  Rename createNewTranslog to fileBasedRecovery (#31508)
  [DOCS] Add code snippet testing in more ML APIs (#31339)
  [DOCS] Remove fixed file from build.gradle
  [DOCS] Creates field and document level security overview (#30937)
  Test: Skip assertion on windows
  [DOCS] Move migration APIs to docs (#31473)
  Add a known issue for upgrading from 5.x to 6.3.0 (#31501)
  Return transport addresses from UnicastHostsProvider (#31426)
  Add Delete Snapshot High Level REST API
  Reload secure settings for plugins (#31481)
  [DOCS] Fix JDBC Maven client group/artifact ID
@tomcallahan tomcallahan added >docs General docs changes and removed :Docs labels Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :ml Machine learning v6.3.1 v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants