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

[ML] stop using isAllowedByLicense for model license checks #79908

Conversation

benwtrent
Copy link
Member

For trained models, there are both basic and platinum licenses allowed.

We should track the feature usage check based on the license operation mode instead of using the underlying isAllowedByLicense.

closes: #79811

@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Oct 27, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@benwtrent
Copy link
Member Author

@rjernst is this required for 8.0.0? Or is 8.1.0 OK?

@rjernst
Copy link
Member

rjernst commented Oct 27, 2021

8.1.0 is fine, there is no rush. This is just a general direction for the license state to simplify it.

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

@benwtrent benwtrent added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 28, 2021
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I have a couple thoughts

@benwtrent benwtrent removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 28, 2021
Co-authored-by: Przemysław Witek <przemyslaw.witek@elastic.co>
@benwtrent benwtrent added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 28, 2021
@elasticsearchmachine elasticsearchmachine merged commit 8a8d868 into elastic:master Oct 28, 2021
@benwtrent benwtrent deleted the feature/remove-ml-xpack-license-method branch October 28, 2021 14:09
@@ -40,6 +41,12 @@
License.OperationMode.PLATINUM
);

public static final LicensedFeature.Momentary ML_MODEL_INFERENCE_PLATINUM_FEATURE = LicensedFeature.momentary(
MachineLearningField.ML_FEATURE_FAMILY,
"model-inference-platinum-check",
Copy link
Member

Choose a reason for hiding this comment

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

@benwtrent Followup thought: this string is meant to be sent to users. Is there something we could use that doesn't have the license level or "check" in it? It's meant to be a short descriptor of what the feature is. How do these two license levels (basic vs platinum) for model inference differ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rjernst you are 100% right here. I thought about this a tad more and simplified our checks.

Since we were doing API license checks (Plat), we only need to verify if the model is basic or not. So, this new feature tracking isn't required.

See here: #80031

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Oct 28, 2021
…formance

* upstream/master: (153 commits)
  [ML] update truncation default & adding field output when input is truncated (elastic#79942)
  [ML] stop using isAllowedByLicense for model license checks (elastic#79908)
  [ML] Retain built-in ML roles granting Kibana privileges (elastic#80014)
  [Transform] remove old mixed cluster BWC layers, not required for 8x (elastic#79927)
  Increase test timeout for CoordinatorTests testAllSearchesExecuted
  [Transform] add rolling upgrade tests for upgrade endpoint (elastic#79721)
  [ML] Update trained model docs for truncate parameter for bert tokenization (elastic#79652)
  `CoordinatorTests` sometimes needs three term bumps (elastic#79574)
  [ML] Account for service being triggered twice in tests (elastic#80000)
  SearchContext: remove unused variable (elastic#79917)
  Revert "Deprecate resolution loss on date field (elastic#78921)" (elastic#79914)
  Re-enable GeoIpDownloaderIT#testStartWithNoDatabases() (elastic#79907)
  Fix SnapshotBasedIndexRecoveryIT#testSeqNoBasedRecoveryIsUsedAfterPrimaryFailOver (elastic#79469)
  Fix RecoverySourceHandlerTests (elastic#79546)
  SQL: stabilize SqlSearchPageTimeoutIT (elastic#79928)
  Wait 3 seconds for the server to reload trust (elastic#79778)
  Skip automatically preserved request headers when rewriting (elastic#79973)
  Check whether stdout is a real console (elastic#79882)
  Convert remote license checker to use LicensedFeature (elastic#79876)
  Miscellaneous fixes for LDAP SDK v6 upgrade (elastic#79891)
  ...

# Conflicts:
#	libs/x-content/src/main/java/org/elasticsearch/xcontent/support/filtering/FilterPath.java
#	libs/x-content/src/test/java/org/elasticsearch/xcontent/support/filtering/FilterPathGeneratorFilteringTests.java
#	libs/x-content/src/test/java/org/elasticsearch/xcontent/support/filtering/FilterPathTests.java
elasticsearchmachine pushed a commit that referenced this pull request Oct 28, 2021
Since trained models require either a platinum or basic license, our
license checking can be simplified.  Now, we check if ML APIs are
allowed (platinum) or if the trained model is `basic`.  relates to:
#79908
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :ml Machine learning >non-issue Team:ML Meta label for the ML team v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove use of XPackLicenseState::isAllowedByLicense for ML models
5 participants