[test] Add integration test for accessing sd sttr in dc

[JiangJiaWei1103]

Tracking issue

flyteorg/flyte#5956

Why are the changes needed?

So far, this patch is tested via single binary, which is limited. The newly released flyte now supports accessing StructuredDataset attribute in a dataclass. Hence, we should add an integration to enhance the robustness.

What changes were proposed in this pull request?

Add an integration test supporting:

1. Upload a single parquet file to minio s3 bucket
2. Access StructuredDataset attribute from a dataclass
3. Open StructuredDataset from a remote path (i.e., the uploaded location)
4. Teardown the uploaded file after workflow is completed

How was this patch tested?

This integration test passes in local run.

Setup process

For local run, the setup process is summarized as follows:

git clone https://github.com/flyteorg/flytekit.git
gh pr checkout 2969
make setup && pip install -e .

After installation, run the following command:

pytest -svvv tests/flytekit/integration/remote/test_remote.py::test_attr_access_sd

Screenshots

Check all the applicable boxes

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

Related PRs

#2954

Docs link

Summary by Bito

This PR implements integration tests for StructuredDataset attribute access within dataclasses in Flyte, featuring a test workflow for parquet file handling in minio s3 bucket. The implementation includes file transfer utilities and comprehensive test cases, demonstrating the complete lifecycle from upload to cleanup.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.53%. Comparing base (92d87a8) to head (fbfac6e).
Report is 33 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2969      +/-   ##
==========================================
- Coverage   82.79%   77.53%   -5.26%     
==========================================
  Files           3      288     +285     
  Lines         186    24695   +24509     
  Branches        0     2729    +2729     
==========================================
+ Hits          154    19148   +18994     
- Misses         32     4786    +4754     
- Partials        0      761     +761     

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

[eapolinario]

A couple of nits, otherwise, looks good.

[JiangJiaWei1103]

Hi @eapolinario,

Thanks a lot for reviewing this. I have one more question!

Would it be better to move the definition of SimpleFileTransfer to another file including common utilities?

[flyte-bot]

Code Review Agent Run #4d8bc5

Actionable Suggestions - 4

• tests/flytekit/integration/remote/utils.py - 1

  • Consider making test data path configurable · Line 89-89
• tests/flytekit/integration/remote/test_remote.py - 3

  • Consider using unittest.mock instead of mock · Line 17-17
  • Consider adding error handling for upload · Line 811-812
  • Consider handling file deletion errors · Line 820-823

Review Details

• Files reviewed - 3 · Commit Range: d5ae883..3fc02ea

  • tests/flytekit/integration/remote/test_remote.py
  • tests/flytekit/integration/remote/utils.py
  • tests/flytekit/integration/remote/workflows/basic/attr_access_sd.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

AI Code Review powered by

[flyte-bot]

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Testing - Integration Test for StructuredDataset Attribute Access	- test_remote.py - Added test case for accessing StructuredDataset attribute from dataclass - utils.py - Created utility class for file transfer operations with minio s3 bucket - attr_access_sd.py - Implemented workflow for testing StructuredDataset attribute access

[flyte-bot]

Consider making test data path configurable

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

Check the AI-generated fix before applying

Suggested change

	tmp_file_path = pathlib.Path(__file__).parent / "workflows/basic/data/df.parquet/00000"
	TEST_DATA_DIR = pathlib.Path(__file__).parent / "test_data"
	tmp_file_path = TEST_DATA_DIR / "df.parquet/00000"

Code Review Run #4d8bc5

---

Is this a valid issue, or was it incorrectly flagged by the Agent?

• it was incorrectly flagged

[flyte-bot]

Consider adding error handling for upload

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

Check the AI-generated fix before applying

Suggested change

	file_transfer = SimpleFileTransfer()
	remote_file_path = file_transfer.upload_file(file_type="parquet")
	file_transfer = SimpleFileTransfer()
	try:
	remote_file_path = file_transfer.upload_file(file_type="parquet")
	except Exception as e:
	# Clean up any partial uploads if needed
	raise RuntimeError(f"Failed to upload file: {str(e)}")

Code Review Run #4d8bc5

---

Is this a valid issue, or was it incorrectly flagged by the Agent?

• it was incorrectly flagged

[flyte-bot]

Consider handling file deletion errors

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

Check the AI-generated fix before applying

Suggested change

	# Delete the remote file to free the space
	url = urlparse(remote_file_path)
	bucket, key = url.netloc, url.path.lstrip("/")
	file_transfer.delete_file(bucket=bucket, key=key)
	# Delete the remote file to free the space
	url = urlparse(remote_file_path)
	bucket, key = url.netloc, url.path.lstrip("/")
	try:
	file_transfer.delete_file(bucket=bucket, key=key)
	except Exception as e:
	warnings.warn(f"Failed to delete remote file: {str(e)}")

Code Review Run #4d8bc5

---

Is this a valid issue, or was it incorrectly flagged by the Agent?

• it was incorrectly flagged

[flyte-bot]

Code Review Agent Run Status

• Limitations and other issues: ❌ Failure - Bito Code Review Agent didn't review this pull request automatically because it exceeded the size limit. No action is needed if you didn't intend for the agent to review it. Otherwise, you can initiate the review by typing /review in a comment below.