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

chore: OpenAIPrompt bug fixes #2334

Merged

Conversation

sss04
Copy link
Contributor

@sss04 sss04 commented Jan 6, 2025

Related Issues/PRs

A couple of bug fixes:

  • move error column to back
  • rename error column
  • fix issue where params that don't exist were being checked

What changes are proposed in this pull request?

Briefly describe the changes included in this Pull Request.

How is this patch tested?

  • I have written tests (not required for typo or doc fix) and confirmed the proposed feature/bug-fix/change works.

Does this PR change any dependencies?

  • No. You can skip this section.
  • Yes. Make sure the dependencies are resolved correctly, and list changes here.

Does this PR add a new feature? If so, have you added samples on website?

  • No. You can skip this section.
  • Yes. Make sure you have added samples following below steps.
  1. Find the corresponding markdown file for your new feature in website/docs/documentation folder.
    Make sure you choose the correct class estimators/transformers and namespace.
  2. Follow the pattern in markdown file and add another section for your new API, including pyspark, scala (and .NET potentially) samples.
  3. Make sure the DocTable points to correct API link.
  4. Navigate to website folder, and run yarn run start to make sure the website renders correctly.
  5. Don't forget to add <!--pytest-codeblocks:cont--> before each python code blocks to enable auto-tests for python samples.
  6. Make sure the WebsiteSamplesTests job pass in the pipeline.

@sss04 sss04 requested a review from mhamilton723 as a code owner January 6, 2025 04:53
@sss04
Copy link
Contributor Author

sss04 commented Jan 6, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.49%. Comparing base (3ec2ccd) to head (3484bb0).

Files with missing lines Patch % Lines
...zure/synapse/ml/services/openai/OpenAIPrompt.scala 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2334      +/-   ##
==========================================
- Coverage   84.52%   84.49%   -0.04%     
==========================================
  Files         328      328              
  Lines       16844    16850       +6     
  Branches     1512     1516       +4     
==========================================
- Hits        14238    14237       -1     
- Misses       2606     2613       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sss04
Copy link
Contributor Author

sss04 commented Jan 6, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

val results = completionNamed
.transform(dfTemplated)
.withColumn(getOutputCol,
getParser.parse(F.element_at(F.col(completionNamed.getOutputCol).getField("choices"), 1)
.getField("text")))
.drop(completionNamed.getOutputCol)

val resultsFinal = results.select(results.columns.filter(_ != getErrorCol).map(col) :+ col(getErrorCol): _*)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this necessary, do error colum,ns show up before other columns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the Error column appears before the output column - we wanted it at the end so I added this

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this change reflected in transform schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After trying modifying SimpleHTTPTransformer, I think the withColumn(getOutputCol on line 192 is moving the output column to the back

Copy link
Contributor Author

@sss04 sss04 Jan 6, 2025

Choose a reason for hiding this comment

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

did some surface level research and it seems if we're rearranging columns, transformSchema doesn't need a change? if that's true, we're good with the code as is

Copy link
Collaborator

Choose a reason for hiding this comment

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

it unfortunately will as it is ordered under the hood, you can ovverride this func too to do the switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed transformSchema to move the Error column to the back

@sss04
Copy link
Contributor Author

sss04 commented Jan 6, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sss04
Copy link
Contributor Author

sss04 commented Jan 7, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mhamilton723 mhamilton723 merged commit ec2c1c1 into microsoft:master Jan 7, 2025
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants