In FlyteRemote.upload_file, pass the file object directly rather than the entire bytes buffer

[redartera]

Why are the changes needed?

In the upload_file function, we read the entire file's content and hold onto it in memory - this is problematic when dealing with large files. Moreover, there is an issue where requests.put can raise an OverflowError exception when dealing with large byte buffers.

For a demo replicating the issue and demonstrating the fix, please refer to this comment.

Setup: Python 3.9, flytekit 1.12.3 and Ubuntu 22.04

What changes were proposed in this pull request?

The proposed changes follow the recommendation from the official requests documentation

Additionally, we add an integration test for FlyteRemote.upload_file that is run separately from the rest of our integration tests, using a new lftransfers pytest marker. Changes were made to our Makefile, our pythonbuild.yml as well as the markers section of pyproject.toml accordingly.

Check all the applicable boxes

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

Related PRs

[redartera]

any recommendations on how this change might be tested in a more systematic way?

[pingsutw]

@redartera Thank you so much for fixing it.

any recommendations on how this change might be tested in a more systematic way?

We can register a task with a large file in the integration test.

flytekit/tests/flytekit/integration/remote/test_remote.py

Lines 31 to 49 in ea6fa0d

	def register():
	out = subprocess.run(
	[
	"pyflyte",
	"--verbose",
	"-c",
	CONFIG,
	"register",
	"--image",
	IMAGE,
	"--project",
	PROJECT,
	"--domain",
	DOMAIN,
	"--version",
	VERSION,
	MODULE_PATH,
	]
	)

btw, this is awesome. I didn't know request supports streaming uploads. It will reduce lots of memory overhead at compile time.

[codecov]

Codecov Report

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

Project coverage is 78.47%. Comparing base (72da0d0) to head (c5075ae).
Report is 4 commits behind head on master.

Files	Patch %	Lines
flytekit/remote/remote.py	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2641      +/-   ##
==========================================
- Coverage   78.77%   78.47%   -0.31%     
==========================================
  Files         187      187              
  Lines       19149    19151       +2     
  Branches     3993     3994       +1     
==========================================
- Hits        15085    15029      -56     
- Misses       3374     3436      +62     
+ Partials      690      686       -4     

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

[redartera]

@redartera Thank you so much for fixing it.

any recommendations on how this change might be tested in a more systematic way?

We can register a task with a large file in the integration test.

flytekit/tests/flytekit/integration/remote/test_remote.py

Lines 31 to 49 in ea6fa0d

	def register():
	out = subprocess.run(
	[
	"pyflyte",
	"--verbose",
	"-c",
	CONFIG,
	"register",
	"--image",
	IMAGE,
	"--project",
	PROJECT,
	"--domain",
	DOMAIN,
	"--version",
	VERSION,
	MODULE_PATH,
	]
	)

btw, this is awesome. I didn't know request supports streaming uploads. It will reduce lots of memory overhead at compile time.

sounds good - I added a test which writes a large file to a module then runs pyflyte register ... on that module.

[redartera]

@redartera Thank you so much for fixing it.

any recommendations on how this change might be tested in a more systematic way?

We can register a task with a large file in the integration test.

flytekit/tests/flytekit/integration/remote/test_remote.py

Lines 31 to 49 in ea6fa0d

	def register():
	out = subprocess.run(
	[
	"pyflyte",
	"--verbose",
	"-c",
	CONFIG,
	"register",
	"--image",
	IMAGE,
	"--project",
	PROJECT,
	"--domain",
	DOMAIN,
	"--version",
	VERSION,
	MODULE_PATH,
	]
	)

btw, this is awesome. I didn't know request supports streaming uploads. It will reduce lots of memory overhead at compile time.

sounds good - I added a test which writes a large file to a module then runs pyflyte register ... on that module.

seems like the GH runners are running out of disk space, I reduced the file size to 2 and 3gb while adding the @pytest.mark.serial marker to avoid running these tests in parallel and overloading the runner's disk.

[eapolinario]

seems like the GH runners are running out of disk space, I reduced the file size to 2 and 3gb while adding the @pytest.mark.serial marker to avoid running these tests in parallel and overloading the runner's disk.

What if we added a separate pytest marker to run just this test?

[redartera]

seems like the GH runners are running out of disk space, I reduced the file size to 2 and 3gb while adding the @pytest.mark.serial marker to avoid running these tests in parallel and overloading the runner's disk.

What if we added a separate pytest marker to run just this test?

I like this approach - will find a cleaner way to adopt it

[redartera]

@eapolinario @pingsutw I made a few major changes to both the implementation and the integration test:

• I now use stat following the recommendation of @eapolinario - works as expected.
• I now use a separate lftransfers pytest marker also following the recommendation of @eapolinario. Changes were made to the Makefile, pythonbuild.yml and pyproject.toml to allow those tests to be ran separately from the rest of the integration tests
• The test itself has changed, where I now simply test the upload_file method, thus making it easier to clean all objects which were created by the test. If i were to register a module and upload 2/3gb files each time, it would be difficult to cleanly delete all artifacts which were generated by each test, and the sandbox volume of users or GH runners would quickly become overloaded if our large files are persisted across test runs.

[eapolinario]

Thank you!