add check fabric setting #2163
Closed
add check fabric setting #2163
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+1
to
+9
| // Tenant setting status check by model name ("GPT-4", "gpt-35-turbo") | ||
| // eg: FabricTenant.getModelStatus("GPT-4") | ||
| // returnType: Boolean | ||
|
|
||
| // Part of the code are scala implementation of | ||
| // synapse.ml.internal_utils.session_utils | ||
| // synapse.ml.fabric.token_utils | ||
| // synapse.ml.mlflow.synapse_mlflow_utils | ||
|
|
There was a problem hiding this comment.
Put this docstring in the class, use the official header to pass the stylecheck
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+25
to
+36
| object StringifiedJsonProtocol extends DefaultJsonProtocol { | ||
| implicit object StringifiedMapFormat extends RootJsonFormat[Map[String, String]] { | ||
| def write(map: Map[String, String]): JsValue = { | ||
| JsObject(map.mapValues(JsString(_))) | ||
| } | ||
|
|
||
| def read(value: JsValue): Map[String, String] = value.asJsObject.fields.map { | ||
| case (key, JsString(str)) => key -> str | ||
| case (key, other) => key -> other.toString | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
import spray.json.DefaultProtocol._
mhamilton723
reviewed
Feb 5, 2024
| def readJsonFileAsMap(filePath: String): Try[Map[String, String]] = { | ||
| Try { | ||
| val fileContent = Source.fromFile(filePath).getLines.mkString | ||
| fileContent.parseJson.convertTo[Map[String, String]]//.asJsObject.fields |
There was a problem hiding this comment.
reminder to remove comment
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+46
to
+49
| val pbiEnv = sc.getConf.get("spark.trident.pbienv").toLowerCase | ||
| val tokenServiceEndpoint = sc.hadoopConfiguration.get("trident.lakehouse.tokenservice.endpoint") | ||
| val capacityId = sc.hadoopConfiguration.get("trident.capacity.id") | ||
| val workspaceId = sc.hadoopConfiguration.get("trident.artifact.workspace.id") |
There was a problem hiding this comment.
make these lazy vals instead of vals
mhamilton723
reviewed
Feb 5, 2024
|
|
||
| class FabricToken { | ||
|
|
||
| def readJsonFileAsMap(filePath: String): Try[Map[String, String]] = { |
There was a problem hiding this comment.
make everytthing private or private[ml] unless you want people to call it
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+39
to
+42
| final val CONTEXT_FILE_PATH = "/home/trusted-service-user/.trident-context" | ||
| final val TOKEN_PATH = "/opt/token-service/tokenservice.config.json" | ||
| final val TRIDENT_LAKEHOUSE_TOKEN_SERVICE_ENDPOINT = "trident.lakehouse.tokenservice.endpoint" | ||
| final val TRIDENT_SESSION_TOKEN = "trident.session.token" |
There was a problem hiding this comment.
Nit on Style: no camelcase in scala, you can just use the regular naming case (with a capital first letter because its an object
mhamilton723
reviewed
Feb 5, 2024
| val host = TargetUrl.map(_.getHost).get | ||
| val path = TargetUrl.map(_.getPath).get | ||
|
|
||
| def getAADToekn(): String = { |
mhamilton723
reviewed
Feb 5, 2024
| } | ||
| } | ||
|
|
||
| object FabricTenant extends FabricToken { |
There was a problem hiding this comment.
Factor out common code shared by this and logging infra
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+144
to
+151
| httpGet.setHeader("Authorization", s"Bearer ${aadToken}") | ||
| val client = HttpClients.createDefault() | ||
| val response = client.execute(httpGet) | ||
| val entity = response.getEntity | ||
| val content = EntityUtils.toString(entity) | ||
| val jsonData = content.parseJson.convertTo[Map[String, String]] | ||
| response.close() | ||
| client.close() |
There was a problem hiding this comment.
replace with the safe HTTP calls like in other code
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+159
to
+164
| val httpPost = new HttpPost(url) | ||
| httpPost.setHeader("Content-type", "application/json") | ||
| httpPost.setHeader("Authorization", s"Bearer ${aadToken}") | ||
|
|
||
| val payload = s"""{"capacityObjectId": "${Configs.capacityId}", "workspaceObjectId": "${Configs.workspaceId}", "workloadType": "${MWC_WORKLOAD_TYPE_ML}"}"""" | ||
| httpPost.setEntity(new StringEntity(payload, "UTF-8")) |
There was a problem hiding this comment.
anytime you make a post you can use existing utility function
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+187
to
+195
| val client = HttpClients.createDefault() | ||
| val url = getMLFlowWorkloadEndpoint + "cognitive/openai/tenantsetting" | ||
| val httpPost = new HttpPost(url) | ||
| httpPost.setHeader("Content-type", "application/json") | ||
| httpPost.setHeader("Authorization", s"Bearer ${aadToken}") | ||
| httpPost.setEntity(new StringEntity(jsonString)) | ||
|
|
||
| val response = client.execute(httpPost) | ||
| val responseString = EntityUtils.toString(response.getEntity) |
mhamilton723
reviewed
Feb 5, 2024
Comment on lines
+199
to
+203
| val responseField = responseString.parseJson.asJsObject.fields.get(modelName.toLowerCase).get | ||
| val resultString: String = responseField match { | ||
| case JsString(value) => value // Directly extract the string value | ||
| } | ||
| // Allowed, Disallowed, DisallowedForCrossGeo, ModelNotFound, InvalidResult |
There was a problem hiding this comment.
i think theres a simpler 1-liner for this
mhamilton723
reviewed
Feb 5, 2024
| mlflowWorkloadEndpoint | ||
| } | ||
|
|
||
| def getModelStatus(modelName: String): Boolean = { |
There was a problem hiding this comment.
maybe
def assertModelReady(modelName): Unit = {
.....
throw new IllegalArgumentException("Useful error message about the status and a link to the URL on how to set up the model endpoint stuff")
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tenant setting status check by model name ("GPT-4", "gpt-35-turbo")
eg: FabricTenant.getModelStatus("GPT-4")
returnType: Boolean
Part of the code are scala implementation of
synapse.ml.internal_utils.session_utils
synapse.ml.fabric.token_utils
synapse.ml.mlflow.synapse_mlflow_utils