-
Notifications
You must be signed in to change notification settings - Fork 858
chore: Add argument to limit file size, put file error to errorCol #2464
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
Conversation
|
Hey @levscaut 👋! 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances file validation in OpenAIPrompt by adding configurable size and extension limits with graceful error handling for file preparation failures.
Changes:
- Added file size limit parameter (
fileSizeLimitMB) to restrict maximum file sizes - Added valid file extensions parameter (
validFileExtensions) to restrict allowed file types - Modified error handling to capture file preparation errors in
errorColinstead of failing the entire process - Enhanced
SimpleHTTPTransformerto preserve upstream errors usingcoalesce
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala | Added file validation parameters and improved error handling for file operations |
| cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptSuite.scala | Added comprehensive tests for new validation features and error handling |
| core/src/main/scala/com/microsoft/azure/synapse/ml/io/http/SimpleHTTPTransformer.scala | Modified to preserve existing errors from upstream transformations |
Comments suppressed due to low confidence (1)
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala:255
- The
localParamNamessequence is missing the newly added parametersfileSizeLimitMBandvalidFileExtensions. These should be included if they are meant to be local parameters that are not passed to the underlying services.
private val localParamNames = Seq(
"promptTemplate", "outputCol", "postProcessing", "postProcessingOptions", "dropPrompt", "dropMessages",
"systemPrompt", "apiType", "usageCol", "responseIdCol")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptSuite.scala
Show resolved
Hide resolved
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPromptSuite.scala
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2464 +/- ##
==========================================
+ Coverage 84.42% 84.44% +0.01%
==========================================
Files 335 335
Lines 17750 17782 +32
Branches 1622 1579 -43
==========================================
+ Hits 14986 15016 +30
- Misses 2764 2766 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/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). |
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Outdated
Show resolved
Hide resolved
|
/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). |
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIPrompt.scala
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ranadeepsingh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for looking at the feedback!
Related Issues/PRs
N/A
What changes are proposed in this pull request?
This PR adds file validation parameters to
OpenAIPromptand improves error handling for file preparation:Graceful error handling for file preparation: Instead of failing the entire process when a file cannot be read (e.g., file not found), errors are now captured in
errorColwith null inoutputCol, allowing other rows to process normally.File size limit (
setFileSizeLimitMB): New parameter to restrict maximum file size in megabytes. Files exceeding this limit produce an error inerrorCol.SimpleHTTPTransformer enhancement: Modified to preserve existing
errorColvalues usingcoalesce, enabling upstream error propagation.How is this patch tested?
New test cases added:
file preparation error stores error in errorCol instead of failingfile size limit produces error when exceededDoes this PR change any dependencies?