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

[House keeping] remove setting max size bytes in node context #5092

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

pvditt
Copy link
Contributor

@pvditt pvditt commented Mar 21, 2024

Tracking issue

#4487

Why are the changes needed?

#4852 deprecated the max-output-size-bytes propeller config in favor of maxDownloadMBs storage config. This made setting MaxSizeBytes in NodeExecutionContext no longer needed as we can just read that config when reading an output file.

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

#4852

Docs link

@pvditt pvditt marked this pull request as ready for review March 21, 2024 19:18
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. housekeeping Issues that help maintain flyte and keep it tech-debt free labels Mar 21, 2024
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 58.98%. Comparing base (2db8eed) to head (b31f638).
Report is 96 commits behind head on master.

Files Patch % Lines
...uginmachinery/ioutils/remote_file_output_reader.go 0.00% 7 Missing ⚠️
flyteplugins/go/tasks/plugins/testing/echo.go 0.00% 1 Missing ⚠️
...yteplugins/go/tasks/plugins/webapi/agent/plugin.go 0.00% 1 Missing ⚠️
...ugins/go/tasks/plugins/webapi/databricks/plugin.go 0.00% 1 Missing ⚠️
flytepropeller/pkg/controller/controller.go 0.00% 1 Missing ⚠️
...ytepropeller/pkg/controller/nodes/array/handler.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5092      +/-   ##
==========================================
- Coverage   58.99%   58.98%   -0.02%     
==========================================
  Files         645      645              
  Lines       55648    55649       +1     
==========================================
- Hits        32831    32826       -5     
- Misses      20222    20228       +6     
  Partials     2595     2595              
Flag Coverage Δ
unittests 58.98% <50.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@pvditt pvditt requested a review from hamersaw March 21, 2024 19:29
return RemoteFileOutputReader{
outPath: outPaths,
store: store,
maxPayloadSize: maxDatasetSize,
maxPayloadSize: maxPayloadSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

is there an issue with just removing the maxPayloadSize completely? IIUC there is just the two checks in this file. IMO the maxDownloadMBs applied on the data store retrieval would catch these first.

Copy link
Contributor Author

@pvditt pvditt Mar 22, 2024

Choose a reason for hiding this comment

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

I should've mentioned this in the PR - I had some concern if there were any swaps with an external datastore implementation as mentioned in this comment. This external implementation might not have checks against maxDownloadMBs as we do in stow_store.

This is probably an unfound concern and we could just mention in release notes about potential payload size issues if you're using an external storage implementation.

Signed-off-by: Paul Dittamo <[email protected]>
@pvditt pvditt requested a review from hamersaw April 3, 2024 18:12
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 19, 2024
@pvditt pvditt merged commit 2ca3111 into master Apr 19, 2024
46 of 48 checks passed
@pvditt pvditt deleted the housekeeping/remove-maxsizebytes branch April 19, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Issues that help maintain flyte and keep it tech-debt free lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants