diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md new file mode 100644 index 0000000000..e906bd2271 --- /dev/null +++ b/.agents/skills/code-review/SKILL.md @@ -0,0 +1,16 @@ +--- +name: code-review +description: Quick review checklist for python and scala code changes before callings it done. +--- + +# Code Review + +Use this skill when reviewing SynapseML changes. + +## Steps + +1. Inspect diff: `git --no-pager diff --stat && git --no-pager diff` +2. Run Scala style: `sbt scalastyle test:scalastyle` +3. Run Python format check: `black --check --extend-exclude 'docs/' .` +4. Run targeted tests for touched code. +5. Report only concrete issues with file paths and fixes. diff --git a/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala b/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala index 18c71d7a34..fbf1d58428 100644 --- a/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala +++ b/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala @@ -381,29 +381,48 @@ class OpenAIPrompt(override val uid: String) extends Transformer (dfWithFilenames, pathColumnNames, filenameColMapping, templateWithFilenameRefs) } - private def validateResponsesApiParams(): Unit = { - val currentApiType = if (isSet(apiType)) getApiType else "chat_completions" + private def resolvedApiType: String = if (isSet(apiType)) getApiType else "chat_completions" - if (currentApiType != "responses") { - if (isSet(store) && getStore) { - throw new IllegalArgumentException( - "store parameter requires apiType='responses'. Use .setApiType(\"responses\")") - } + private def hasPreviousResponseIdConfigured: Boolean = { + get(previousResponseId).orElse(getDefault(previousResponseId)).isDefined + } - if (get(previousResponseId).orElse(getDefault(previousResponseId)).isDefined) { - throw new IllegalArgumentException( - "previousResponseId requires apiType='responses'. Use .setApiType(\"responses\")") - } + private def validateResponsesApiCompatibility(currentApiType: String): Unit = { + if (currentApiType == "responses" && hasAIFoundryModel) { + throw new IllegalArgumentException( + "apiType='responses' is not supported with AI Foundry chat endpoints. " + + "Use .setApiType(\"chat_completions\") or configure an OpenAI endpoint with deploymentName.") + } + } + + private def validateResponsesOnlyParams(currentApiType: String): Unit = { + if (currentApiType != "responses" && isSet(store) && getStore) { + throw new IllegalArgumentException( + "store parameter requires apiType='responses'. Use .setApiType(\"responses\")") } + if (currentApiType != "responses" && hasPreviousResponseIdConfigured) { + throw new IllegalArgumentException( + "previousResponseId requires apiType='responses'. Use .setApiType(\"responses\")") + } + } + + private def validateUsageColSupport(currentApiType: String): Unit = { if (isSet(usageCol) && !hasAIFoundryModel && - getApiType != "chat_completions" && getApiType != "responses") { + currentApiType != "chat_completions" && currentApiType != "responses") { throw new IllegalArgumentException( s"usageCol not supported for apiType='$currentApiType'. " + - "Use 'chat_completions', 'responses', or AI Foundry chat APIs.") + "Use 'chat_completions', 'responses', or AI Foundry chat APIs.") } } + private def validateResponsesApiParams(): Unit = { + val currentApiType = resolvedApiType + validateResponsesApiCompatibility(currentApiType) + validateResponsesOnlyParams(currentApiType) + validateUsageColSupport(currentApiType) + } + override def transform(dataset: Dataset[_]): DataFrame = { transferGlobalParamsToParamMap() validateResponsesApiParams() @@ -613,16 +632,21 @@ class OpenAIPrompt(override val uid: String) extends Transformer else "unsupported" } - private[openai] def hasAIFoundryModel: Boolean = this.isDefined(model) + private def isAIFoundryEndpoint: Boolean = { + val host = get(url).orElse(getDefault(url)).flatMap { raw => + Try(new URI(raw).getHost).toOption.orElse(Try(new URL(raw).getHost).toOption) + } + host.exists(_.toLowerCase.endsWith("services.ai.azure.com")) + } + + private[openai] def hasAIFoundryModel: Boolean = this.isDefined(model) && isAIFoundryEndpoint //deployment name can be set by user, it doesn't have to match with model name private def getOpenAIChatService: OpenAIServicesBase with HasTextOutput = { - val completion: OpenAIServicesBase with HasTextOutput = if (hasAIFoundryModel) { new AIFoundryChatCompletion() - } - else { + } else { // Use the apiType parameter to decide between chat_completions and responses getApiType match { case "responses" => new OpenAIResponses() @@ -634,6 +658,13 @@ class OpenAIPrompt(override val uid: String) extends Transformer .filter(p => !localParamNames.contains(p.param.name) && completion.hasParam(p.param.name)) .foreach(p => completion.set(completion.getParam(p.param.name), p.value)) + completion match { + case resp: OpenAIResponses + if this.isDefined(model) && get(deploymentName).orElse(getDefault(deploymentName)).isEmpty => + resp.setDeploymentName(getModel) + case _ => + } + completion } diff --git a/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponses.scala b/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponses.scala index 454cff3674..d9f25896ab 100644 --- a/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponses.scala +++ b/cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponses.scala @@ -224,19 +224,43 @@ class OpenAIResponses(override val uid: String) extends OpenAIServicesBase(uid) } override private[openai] def getOutputMessageText(outputColName: String): org.apache.spark.sql.Column = { - F.element_at(F.element_at(F.col(outputColName).getField("output"), 1) - .getField("content"), 1).getField("text") + val outputEntries = F.col(outputColName).getField("output") + val lastOutputEntry = F.element_at(outputEntries, -1) + val textValues = F.transform(lastOutputEntry.getField("content"), part => part.getField("text")) + val definedTextValues = F.filter(textValues, text => text.isNotNull) + F.element_at(definedTextValues, 1) + } + + private def outputEntries(outputRow: Row): Seq[Row] = { + Option(outputRow).flatMap(r => Option(r.getAs[Seq[Row]]("output"))).getOrElse(Seq.empty) + } + + private def lastOutputEntry(outputRow: Row): Option[Row] = { + outputEntries(outputRow).lastOption + } + + private def firstDefinedText(contentParts: Seq[Row]): Option[String] = { + contentParts.iterator + .flatMap(part => Option(part.getAs[String]("text"))) + .toSeq + .headOption + } + + private def lastOutputText(outputRow: Row): Option[String] = { + lastOutputEntry(outputRow).flatMap { outputEntry => + firstDefinedText(Option(outputEntry.getAs[Seq[Row]]("content")).getOrElse(Seq.empty)) + } } override private[openai] def isContentFiltered(outputRow: Row): Boolean = { - val result = ResponsesModelResponse.makeFromRowConverter(outputRow) - val firstOutput = result.output.head - Option(firstOutput.content).isEmpty + lastOutputText(outputRow).isEmpty } override private[openai] def getFilterReason(outputRow: Row): String = { - val result = ResponsesModelResponse.makeFromRowConverter(outputRow) - result.output.head.status + lastOutputEntry(outputRow).iterator + .flatMap(outputEntry => Option(outputEntry.getAs[String]("status"))) + .find(_.nonEmpty) + .getOrElse("content_filtered_or_empty") } } diff --git a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptResponsesSuite.scala b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptResponsesSuite.scala new file mode 100644 index 0000000000..54fc054665 --- /dev/null +++ b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptResponsesSuite.scala @@ -0,0 +1,72 @@ +// Copyright (C) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE in project root for information. + +package com.microsoft.azure.synapse.ml.services.openai + +import com.microsoft.azure.synapse.ml.Secrets.getAccessToken +import com.microsoft.azure.synapse.ml.core.test.base.Flaky +import org.apache.spark.sql.DataFrame + +class OpenAIPromptResponsesSuite extends Flaky with OpenAIAPIKey { + + import spark.implicits._ + + override def beforeAll(): Unit = { + val aadToken = getAccessToken("https://cognitiveservices.azure.com/") + println(s"Triggering token creation early ${aadToken.length}") + super.beforeAll() + } + + lazy val df: DataFrame = Seq( + ("apple", "fruits"), + ("mercedes", "cars"), + ("cake", "dishes") + ).toDF("text", "category") + + private def responsesPrompt(outputCol: String, deployment: String): OpenAIPrompt = { + new OpenAIPrompt() + .setSubscriptionKey(openAIAPIKey) + .setDeploymentName(deployment) + .setCustomServiceName(openAIServiceName) + .setApiType("responses") + .setApiVersion("2025-04-01-preview") + .setOutputCol(outputCol) + .setTemperature(0) + } + + private def assertResponsesOutputForDeployment( + deployment: String, + outputCol: String, + expectedToken: String): Unit = { + val prompt = responsesPrompt(outputCol, deployment) + .setPromptTemplate(s"Return exactly the word $expectedToken for {text}.") + + if (deployment.toLowerCase.contains("gpt-5")) { + prompt.setReasoningEffort("low") + prompt.setVerbosity("low") + } + + val output = prompt.transform(df.limit(1)) + .select(outputCol) + .collect() + .head + .getString(0) + + assert(output != null) + assert(output.toLowerCase.contains(expectedToken.toLowerCase)) + } + + test("Responses API OpenAIPrompt returns text for gpt-4.1 outputs") { + assertResponsesOutputForDeployment( + deploymentName4p1, + "responses_gpt41_output", + "fruit") + } + + test("Responses API OpenAIPrompt returns text for gpt-5 outputs") { + assertResponsesOutputForDeployment( + deploymentName5, + "responses_gpt5_output", + "fruit") + } +} diff --git a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptSuite.scala b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptSuite.scala index 15b0d67620..c4e3a0f158 100644 --- a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptSuite.scala +++ b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptSuite.scala @@ -6,12 +6,15 @@ package com.microsoft.azure.synapse.ml.services.openai import com.microsoft.azure.synapse.ml.Secrets.{AIFoundryApiKey, getAccessToken} import com.microsoft.azure.synapse.ml.core.test.base.Flaky import com.microsoft.azure.synapse.ml.core.test.fuzzing.{TestObject, TransformerFuzzing} +import org.apache.http.entity.AbstractHttpEntity +import org.apache.http.util.EntityUtils import org.apache.spark.ml.util.MLReadable import org.apache.spark.sql.{DataFrame, Row} import org.apache.spark.sql.functions.{col, lit} import org.apache.spark.sql.types.{ArrayType, StringType, StructType} import org.scalactic.Equality import com.microsoft.azure.synapse.ml.services.aifoundry.AIFoundryAPIKey +import spray.json._ import java.nio.charset.StandardCharsets import java.nio.file.Files @@ -144,7 +147,7 @@ class OpenAIPromptSuite extends TransformerFuzzing[OpenAIPrompt] with OpenAIAPIK assert(nonNullCount == 3) } - test("Basic Usage Responses API") { + test("Basic Usage Responses API") { val nonNullCount = prompt .setPromptTemplate("give me a comma separated list of 5 {category}, starting with {text} ") .setApiType("responses") @@ -576,6 +579,36 @@ class OpenAIPromptSuite extends TransformerFuzzing[OpenAIPrompt] with OpenAIAPIK assert(p.getPreviousResponseIdCol == "prev_id_column") } + test("responses payload maps model to model field and nests verbosity/reasoning for gpt-5 usage") { + val p = new OpenAIPrompt() + .setApiType("responses") + .setMessagesCol("messages") + .setModel("gpt-5-mini") + .setVerbosity("low") + .setReasoningEffort("low") + .setStore(false) + + val requestRow = Seq( + Seq(OpenAIMessage("user", "Describe what you see in this image.")) + ).toDF("messages").collect().head + + val prepareEntity = classOf[OpenAIPrompt].getDeclaredMethod("prepareEntity") + prepareEntity.setAccessible(true) + val buildEntity = prepareEntity.invoke(p).asInstanceOf[Row => Option[AbstractHttpEntity]] + + val payloadJson = EntityUtils.toString(buildEntity(requestRow).get).parseJson.asJsObject + + assert(payloadJson.fields.contains("input")) + assert(!payloadJson.fields.contains("messages")) + assert(payloadJson.fields.get("model").contains(JsString("gpt-5-mini"))) + + val text = payloadJson.fields("text").asJsObject + assert(text.fields.get("verbosity").contains(JsString("low"))) + + val reasoning = payloadJson.fields("reasoning").asJsObject + assert(reasoning.fields.get("effort").contains(JsString("low"))) + } + test("store=true with Responses API returns response in outputCol and id in auto-generated responseIdCol") { val storePrompt = new OpenAIPrompt() .setSubscriptionKey(openAIAPIKey) diff --git a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponsesSuite.scala b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponsesSuite.scala index e07fd57033..f3cffb1268 100644 --- a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponsesSuite.scala +++ b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIResponsesSuite.scala @@ -7,6 +7,7 @@ import com.microsoft.azure.synapse.ml.core.test.base.Flaky import com.microsoft.azure.synapse.ml.core.test.fuzzing.{TestObject, TransformerFuzzing} import org.apache.http.util.EntityUtils import org.apache.spark.ml.util.MLReadable +import org.apache.spark.sql.{functions => F} import org.apache.spark.sql.{DataFrame, Row} import org.apache.spark.sql.catalyst.expressions.GenericRowWithSchema import org.apache.spark.sql.types.{ArrayType, MapType, StringType, StructField, StructType} @@ -275,6 +276,163 @@ class OpenAIResponsesSuite extends TransformerFuzzing[OpenAIResponses] assert(secondPartType == "input_file") } + test("Responses extract output text from the last output item when reasoning appears first") { + val transformer = new OpenAIResponses() + val responseJson = + """{ + | "id":"resp_test", + | "object":"response", + | "created_at":"1", + | "model":"gpt-5", + | "output":[ + | {"type":"reasoning","status":"completed","content":null}, + | {"type":"message","status":"completed","content":[{"type":"output_text","text":"{\"answer\":\"fruit\"}"}]} + | ], + | "system_fingerprint":null, + | "usage":null + |}""".stripMargin + + val responseDf = spark.read.schema(ResponsesModelResponse.schema).json(Seq(responseJson).toDS) + val wrappedDf = responseDf.select(F.struct(responseDf.columns.map(F.col): _*).as("out")) + + val text = wrappedDf + .select(transformer.getOutputMessageText("out").as("text")) + .collect() + .head + .getAs[String]("text") + + assert(text == """{"answer":"fruit"}""") + assert(!transformer.isContentFiltered(responseDf.collect().head)) + } + + test("Responses extract output text from gpt-4.1 message output") { + val transformer = new OpenAIResponses() + val responseJson = + """{ + | "id":"resp_test", + | "object":"response", + | "created_at":"1", + | "model":"gpt-4.1", + | "output":[ + | {"type":"message","status":"completed","content":[{"type":"output_text","text":"{\"answer\":\"fruit\"}"}]} + | ], + | "system_fingerprint":null, + | "usage":null + |}""".stripMargin + + val responseDf = spark.read.schema(ResponsesModelResponse.schema).json(Seq(responseJson).toDS) + val wrappedDf = responseDf.select(F.struct(responseDf.columns.map(F.col): _*).as("out")) + + val text = wrappedDf + .select(transformer.getOutputMessageText("out").as("text")) + .collect() + .head + .getAs[String]("text") + + assert(text == """{"answer":"fruit"}""") + assert(!transformer.isContentFiltered(responseDf.collect().head)) + } + + test("Responses preserve empty text output in both paths") { + val transformer = new OpenAIResponses() + val responseJson = + """{ + | "id":"resp_test", + | "object":"response", + | "created_at":"1", + | "model":"gpt-5", + | "output":[ + | {"type":"message","status":"completed","content":[{"type":"output_text","text":""}]} + | ], + | "system_fingerprint":null, + | "usage":null + |}""".stripMargin + + val responseDf = spark.read.schema(ResponsesModelResponse.schema).json(Seq(responseJson).toDS) + val wrappedDf = responseDf.select(F.struct(responseDf.columns.map(F.col): _*).as("out")) + + val textRow = wrappedDf + .select(transformer.getOutputMessageText("out").as("text")) + .collect() + .head + + assert(textRow.getAs[String]("text") == "") + assert(!transformer.isContentFiltered(responseDf.collect().head)) + } + + test("Responses prefer text from the last output item when multiple messages exist") { + val transformer = new OpenAIResponses() + val responseJson = + """{ + | "id":"resp_test", + | "object":"response", + | "created_at":"1", + | "model":"gpt-5", + | "output":[ + | {"type":"message","status":"completed","content":[{"type":"output_text","text":"{\"answer\":\"stale\"}"}]}, + | {"type":"message","status":"completed","content":[{"type":"output_text","text":"{\"answer\":\"final\"}"}]} + | ], + | "system_fingerprint":null, + | "usage":null + |}""".stripMargin + + val responseDf = spark.read.schema(ResponsesModelResponse.schema).json(Seq(responseJson).toDS) + val wrappedDf = responseDf.select(F.struct(responseDf.columns.map(F.col): _*).as("out")) + + val text = wrappedDf + .select(transformer.getOutputMessageText("out").as("text")) + .collect() + .head + .getAs[String]("text") + + assert(text == """{"answer":"final"}""") + } + + test("Responses content filtering keeps status reason when no text exists") { + val transformer = new OpenAIResponses() + val responseJson = + """{ + | "id":"resp_test", + | "object":"response", + | "created_at":"1", + | "model":"gpt-5", + | "output":[ + | {"type":"message","status":"content_filter","content":null} + | ], + | "system_fingerprint":null, + | "usage":null + |}""".stripMargin + + val responseDf = spark.read.schema(ResponsesModelResponse.schema).json(Seq(responseJson).toDS) + val outputRow = responseDf.collect().head + + assert(transformer.isContentFiltered(outputRow)) + assert(transformer.getFilterReason(outputRow) == "content_filter") + } + + test("Responses content filtering uses the last output item status reason") { + val transformer = new OpenAIResponses() + val responseJson = + """{ + | "id":"resp_test", + | "object":"response", + | "created_at":"1", + | "model":"gpt-5", + | "output":[ + | {"type":"reasoning","status":"completed","content":null}, + | {"type":"message","status":"content_filter","content":null} + | ], + | "system_fingerprint":null, + | "usage":null + |}""".stripMargin + + val responseDf = spark.read.schema(ResponsesModelResponse.schema).json(Seq(responseJson).toDS) + val outputRow = responseDf.collect().head + + assert(transformer.isContentFiltered(outputRow)) + assert(transformer.getFilterReason(outputRow) == "content_filter") + } + private def testResponses(model: OpenAIResponses, df: DataFrame, requiredLength: Int = 10): Unit = { diff --git a/project/CodegenPlugin.scala b/project/CodegenPlugin.scala index d4cb5c5183..2395ec4926 100644 --- a/project/CodegenPlugin.scala +++ b/project/CodegenPlugin.scala @@ -28,6 +28,19 @@ object CodegenPlugin extends AutoPlugin { override def requires: Plugins = CondaPlugin + private def findBuiltPythonWheel(packageDir: File, projectName: String): File = { + val wheelPrefix = s"${projectName.replace("-", "_")}-" + val wheels = Option(packageDir.listFiles()).getOrElse(Array.empty).filter { file => + file.isFile && + file.getName.startsWith(wheelPrefix) && + file.getName.endsWith(".whl") + }.sortBy(_.lastModified()) + + wheels.lastOption.getOrElse { + throw new IllegalStateException(s"No built wheel found in ${packageDir.getAbsolutePath} for $projectName") + } + } + def rCmd(activateCondaEnv: Seq[String], cmd: Seq[String], wd: File, libPath: String): Unit = { runCmd(activateCondaEnv ++ cmd, wd, Map("R_LIBS" -> libPath, "R_USER_LIBS" -> libPath)) } @@ -236,11 +249,17 @@ object CodegenPlugin extends AutoPlugin { codegen.value createCondaEnvTask.value val destPyDir = join(targetDir.value, "classes", genPackageNamespace.value) - val packageDir = join(codegenDir.value, "package", "python").absolutePath + val packageDirFile = join(codegenDir.value, "package", "python") + val packageDir = packageDirFile.absolutePath val pythonSrcDir = join(codegenDir.value, "src", "python") if (destPyDir.exists()) FileUtils.forceDelete(destPyDir) val sourcePyDir = join(pythonSrcDir.getAbsolutePath, genPackageNamespace.value) FileUtils.copyDirectory(sourcePyDir, destPyDir) + Option(packageDirFile.listFiles()).getOrElse(Array.empty).foreach { file => + if (file.isFile && file.getName.startsWith(s"${name.value.replace("-", "_")}-") && file.getName.endsWith(".whl")) { + FileUtils.forceDelete(file) + } + } packagePythonWheelCmd(packageDir, pythonSrcDir) }, removePipPackage := { @@ -250,23 +269,27 @@ object CodegenPlugin extends AutoPlugin { val packagePythonResult: Unit = packagePython.value val publishLocalResult: Unit = (publishLocal dependsOn packagePython).value val rootPublishLocalResult: Unit = (LocalRootProject / Compile / publishLocal).value + val packageDir = join(codegenDir.value, "package", "python") + val wheel = findBuiltPythonWheel(packageDir, name.value) runCmd( activateCondaEnv ++ Seq( "pip", "install", "-I", "--no-deps", - s"${name.value.replace("-", "_")}-${pythonizedVersion(version.value)}-py2.py3-none-any.whl" + wheel.getAbsolutePath ), - join(codegenDir.value, "package", "python")) + packageDir) }, publishPython := { val packagePythonResult: Unit = packagePython.value val publishLocalResult: Unit = (publishLocal dependsOn packagePython).value val rootPublishLocalResult: Unit = (LocalRootProject / Compile / publishLocal).value - val fn = s"${name.value.replace("-", "_")}-${pythonizedVersion(version.value)}-py2.py3-none-any.whl" + val packageDir = join(codegenDir.value, "package", "python") + val wheel = findBuiltPythonWheel(packageDir, name.value) + val fn = wheel.getName singleUploadToBlob( - join(codegenDir.value, "package", "python", fn).toString, + wheel.toString, version.value + "/" + fn, "pip") }, mergePyCode := { diff --git a/project/build.scala b/project/build.scala index 42f37bce28..74ea0f8d7c 100644 --- a/project/build.scala +++ b/project/build.scala @@ -72,6 +72,9 @@ object BuildUtils { def activateCondaEnv: Seq[String] = { if (sys.props("os.name").toLowerCase.contains("windows")) { osPrefix ++ Seq("activate", condaEnvName, "&&") + } else if (sys.env.get("CONDA_DEFAULT_ENV").contains(condaEnvName)) { + // Avoid nesting `conda run` inside an already activated environment. + Seq() } else { Seq("conda", "run", "-n", condaEnvName, "--no-capture-output") //TODO figure out why this doesn't work