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.
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
[test] Add integration test for accessing sd sttr in dc #2969
[test] Add integration test for accessing sd sttr in dc #2969
Changes from 10 commits
d5ae883
e8abb4a
7937169
fbfac6e
865c390
b640eb6
c27c7f3
a02ea53
83975d0
3fc02ea
838153d
be68c2d
148599b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Consider adding error handling around the file upload operation since network operations can fail. The
upload_file()
call could throw exceptions that should be caught and handled appropriately. A similar issue was also found in tests/flytekit/integration/remote/workflows/basic/attr_access_sd.py (line 34).Code suggestion
Code Review Run #4d8bc5
Is this a valid issue, or was it incorrectly flagged by the Agent?
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.
Consider adding error handling around the file deletion operation. The
delete_file()
call could fail and should be handled gracefully, possibly with a warning if cleanup fails.Code suggestion
Code Review Run #4d8bc5
Is this a valid issue, or was it incorrectly flagged by the Agent?
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.
The hardcoded path
workflows/basic/data/df.parquet/00000
may cause issues if the file structure changes. Consider making this configurable or using a more robust path resolution approach.Code suggestion
Code Review Run #4d8bc5
Is this a valid issue, or was it incorrectly flagged by the Agent?