stripping metadata from dataclass types (#626)#6177
Merged
Future-Outlier merged 2 commits intomasterfrom Jan 17, 2025
Merged
Conversation
* stripping metadata from dataclass types Signed-off-by: Daniel Rammer <hamersaw@protonmail.com> * actually setting on Structure Signed-off-by: Daniel Rammer <hamersaw@protonmail.com> * added unit test Signed-off-by: Daniel Rammer <hamersaw@protonmail.com> --------- Signed-off-by: Daniel Rammer <hamersaw@protonmail.com>
Collaborator
Code Review Agent Run #59e151Actionable Suggestions - 1
Review Details
AI Code Review powered by |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6177 +/- ##
=======================================
Coverage 37.05% 37.06%
=======================================
Files 1318 1318
Lines 132638 132644 +6
=======================================
+ Hits 49151 49163 +12
+ Misses 79237 79231 -6
Partials 4250 4250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Collaborator
Changelist by BitoThis pull request implements the following key changes.
|
flyte-bot
reviewed
Jan 17, 2025
Comment on lines
+262
to
+271
| Structure: &core.TypeStructure{ | ||
| DataclassType: map[string]*core.LiteralType{ | ||
| "foo": { | ||
| Type: &core.LiteralType_Simple{ | ||
| Simple: core.SimpleType_STRING, | ||
| }, | ||
| }, | ||
| }, | ||
| Tag: "str", | ||
| }, |
Collaborator
There was a problem hiding this comment.
Consider simplifying nested structure initialization
Consider simplifying the nested structure by using a more concise initialization pattern for TypeStructure. The current nested structure with multiple levels of indentation could be made more readable. A similar issue was also found in flytepropeller/pkg/compiler/transformers/k8s/utils_test.go (line 227-245).
Code suggestion
Check the AI-generated fix before applying
// Add helper function
func createTypeStructure(key string, simpleType core.SimpleType, tag string) *core.TypeStructure {
return &core.TypeStructure{
DataclassType: map[string]*core.LiteralType{
key: {
Type: &core.LiteralType_Simple{Simple: simpleType},
},
},
Tag: tag,
}
}
// Use helper function in test
Structure: createTypeStructure("foo", core.SimpleType_STRING, "str"),
Code Review Run #59e151
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Daniel Rammer <hamersaw@protonmail.com>
Collaborator
Code Review Agent Run #03ee88Actionable Suggestions - 0Additional Suggestions - 1
Review Details
AI Code Review powered by |
Future-Outlier
approved these changes
Jan 17, 2025
Future-Outlier
deleted the
union/upstream-fb9dce3257974eea378328f2ab78a26c644adeaf
branch
EngHabu
pushed a commit
that referenced
this pull request
Mar 28, 2025
…#631) ## Overview Fixing the[ lint errors](#6177) from upstreaming [this PR](https://github.com/unionai/flyte/pull/626). This means that OSS and the fork remain consistent. ## Test Plan This was tested as part of the original work. ## Rollout Plan (if applicable) This PR does not effect functionality at all, it is purely cosmetic. ## Upstream Changes Should this change be upstreamed to OSS (flyteorg/flyte)? If not, please uncheck this box, which is used for auditing. Note, it is the responsibility of each developer to actually upstream their changes. See [this guide](https://unionai.atlassian.net/wiki/spaces/ENG/pages/447610883/Flyte+-+Union+Cloud+Development+Runbook/#When-are-versions-updated%3F). - [ ] To be upstreamed to OSS ## Issue https://linear.app/unionai/issue/COR-2720/strip-metadata-from-dataclass-types-in-flyteworkflow-crd-interfaces ## Checklist * [ ] Added tests * [ ] Ran a deploy dry run and shared the terraform plan * [ ] Added logging and metrics * [ ] Updated [dashboards](https://unionai.grafana.net/dashboards) and [alerts](https://unionai.grafana.net/alerting/list) * [ ] Updated documentation
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Tracking issue
N/A
Why are the changes needed?
Metadata is stripped from all other places in the interface(s). It is unnecessary for type checking and only attributes to bloating the CRD.
What changes were proposed in this pull request?
Stripping metadata from dataclass types.
How was this patch tested?
Tested under a variety of workflows and
Labels
Setup process
N/A
Screenshots
N/A
Check all the applicable boxes
Related PRs
N/A
Docs link
N/A
Summary by Bito
Implementation of metadata stripping functionality for dataclass types in Flyte compiler, modifying StripTypeMetadata function to handle dataclass types and replacing direct struct field access with getter methods. The changes focus on removing unnecessary metadata and reducing CRD bloat while maintaining type safety, with updates to test coverage to verify the functionality.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1