feat: add aad authentication support for cognitive services #1778
Conversation
|
Hey @serena-ruan 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #1778 +/- ##
===========================================
- Coverage 86.08% 76.08% -10.01%
===========================================
Files 278 278
Lines 14722 14750 +28
Branches 767 763 -4
===========================================
- Hits 12674 11222 -1452
- Misses 2048 3528 +1480
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/cognitive/CognitiveServiceBase.scala
Outdated
Show resolved
Hide resolved
| @@ -132,14 +132,78 @@ trait HasSubscriptionKey extends HasServiceParams { | |||
|
|
|||
| def getSubscriptionKey: String = getScalarParam(subscriptionKey) | |||
|
|
|||
| def setSubscriptionKey(v: String): this.type = setScalarParam(subscriptionKey, v) | |||
| def setSubscriptionKey(v: String): this.type = { | |||
| println("WARNING: Please use setSubscriptionKey together with setLocation for authentication") | |||
There was a problem hiding this comment.
So we will always emit "WARNING" if they set this? No other way to give this information?
There was a problem hiding this comment.
Do you have better ideas? Because most users are quite confused what methods should be used together, like many of them want to setUrl but that's not what we want, we should find an elegant way to warn them.
There was a problem hiding this comment.
you know the code better than me. :) As a user, I get annoyed by WARNINGS in my logs even if I'm using it right.
My only thoughts:
- add this info to doc strings? Or are the HasX traits too generic to do that? Can you override doc strings or params if you are specifying a required combination of params needed?
- validate at transform() time and throw if a proper combination is not set? Is that possible? As a user, this would be useful to me, just to see an error telling me what I did wrong if I didn't set it correctly.
There was a problem hiding this comment.
Yea I totally agree, it might be annoying. Then I found out that for different cog services the usages are also not consistent, like most of them expects setLocation, but there're some expecting setServiceName instead, and for some other cases like DEP they might have to setUrl explicitly. In this case I'll remove the change in this PR and maybe in the future come up with better documentation in general.
There was a problem hiding this comment.
If you want to warn, use the official log4j warning APIs please
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@mhamilton723 Could you review this? 😺 |
|
|
||
| def getSubscriptionKeyCol: String = getVectorParam(subscriptionKey) | ||
|
|
||
| def setSubscriptionKeyCol(v: String): this.type = setVectorParam(subscriptionKey, v) | ||
|
|
||
| } | ||
|
|
||
| trait HasAADToken extends HasServiceParams { | ||
| val aadToken = new ServiceParam[String]( |
There was a problem hiding this comment.
Nit: AAD might be better than aad here
There was a problem hiding this comment.
Looks awesome! We should add a check for this trait in fizzing tests (can do this later). Anything with has subscription key trait should have aad trait too. Also I think we should test this with an aad token we generate from credentials we keep in a key vault or something
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Related Issues/PRs
#xxx
What changes are proposed in this pull request?
Briefly describe the changes included in this Pull Request.
How is this patch tested?
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
website/docs/documentationfolder.Make sure you choose the correct class
estimators/transformersand namespace.DocTablepoints to correct API link.yarn run startto make sure the website renders correctly.<!--pytest-codeblocks:cont-->before each python code blocks to enable auto-tests for python samples.WebsiteSamplesTestsjob pass in the pipeline.