Tracks progress for package creation, upload and kickoff #2935

kumare3 · 2024-11-16T23:38:14Z

Allows users to understand through visualization if large packages are being uploaded or incorrect files are being packaged.

Summary by Bito

This PR implements progress tracking capabilities in Flytekit, adding visual feedback for package uploads and operations through a new environment variable 'FLYTE_SDK_DISPLAY_PROGRESS'. The changes include package creation and compression progress visualization, along with code formatting improvements for better maintainability.

Unit tests added: False

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

Signed-off-by: Ketan Umare <[email protected]>

codecov · 2024-11-17T00:05:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.26%. Comparing base (3f0ab84) to head (59b6742).
Report is 7 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2935       +/-   ##
===========================================
+ Coverage   76.33%   93.26%   +16.93%     
===========================================
  Files         199       48      -151     
  Lines       20840     1842    -18998     
  Branches     2681        0     -2681     
===========================================
- Hits        15908     1718    -14190     
+ Misses       4214      124     -4090     
+ Partials      718        0      -718

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

Mecoli1219

I think we could consider implementing a global Progress to:

Simplify the codebase
Enable consistent progress reporting behavior across modules
Make it easier to add progress tracking to new modules
Allow for a global flag to enable/disable all progress logging (especially if we want something like --verbose or --silence)

eapolinario · 2024-11-18T14:56:17Z

flytekit/remote/remote.py

-                    f"Request to send data {upload_location.signed_url} failed.\nResponse: {rsp.text}",
+        from rich.progress import Progress, TextColumn, TimeElapsedColumn
+
+        progress = Progress(


Progress also has a start method, which is supposed to be invoked explicitly if the Progress object is not used as a context manager (as mentioned by the author here).

We can use this to make the progress bar customizable.

@eapolinario progress is used as a context manager. look at like 1074

Yeah, I understand, but it doesn't have to be used as a context manager, right? If start is not called the progress bars don't show up.

Signed-off-by: Ketan Umare <[email protected]>

Signed-off-by: Jan Fiedler <[email protected]>

fiedlerNr9 · 2025-01-23T04:58:18Z

fast-register.mov

Changes I made:

introduced FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR to control the progress display
updated rich progress task description and looks for creating tarball- & compressing tarball step

flyte-bot · 2025-01-23T05:17:11Z

Code Review Agent Run #941fb7

Actionable Suggestions - 5

flytekit/remote/remote.py - 3
- Consider encapsulating task identifier parameters · Line 446-452
- Consider using dataclass for parameters · Line 613-617
- Consider consolidating progress bar initialization logic · Line 1252-1258
flytekit/tools/fast_registration.py - 1
- Consider more descriptive variable names · Line 170-171
flytekit/loggers.py - 1
- Consider type conversion for env var · Line 191-191

Additional Suggestions - 2

flytekit/remote/remote.py - 2
- Consider combining optional string parameters · Line 613-617
- Consider combining optional parameters into line · Line 479-483

Review Details

Files reviewed - 3 · Commit Range: 59b6742..5b05419
- flytekit/loggers.py
- flytekit/remote/remote.py
- flytekit/tools/fast_registration.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 · 2025-01-23T05:25:44Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Progress Tracking for Package Operations	- `loggers.py` - Added environment variable and function to control progress display - `remote.py` - Implemented progress tracking for package uploads and code formatting improvements - `fast_registration.py` - Added progress visualization for package creation and compression

flyte-bot · 2025-01-23T05:25:46Z

flytekit/remote/remote.py

+    def fetch_task(
+        self,
+        project: str = None,
+        domain: str = None,
+        name: str = None,
+        version: str = None,
+    ) -> FlyteTask:


Consider encapsulating task identifier parameters

Consider using a dataclass or named tuple for the parameters since they are used together in multiple places in the codebase (e.g., fetch_task_lazy, execute_local_task, sync_execution). This could improve code maintainability and reduce parameter duplication.

Code suggestion

Check the AI-generated fix before applying

# Add TaskIdentifier class @dataclass class TaskIdentifier: project: Optional[str] = None domain: Optional[str] = None name: Optional[str] = None version: Optional[str] = None # Update method signature def fetch_task(self, task_id: TaskIdentifier = None) -> FlyteTask:

Code Review Run #941fb7

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

it was incorrectly flagged

flyte-bot · 2025-01-23T05:25:47Z

flytekit/remote/remote.py

+        self,
+        project: str = None,
+        domain: str = None,
+        name: str = None,
+        version: str = None,


Consider using dataclass for parameters

Consider using a dataclass or named tuple for the parameters since they are all optional string parameters that appear to be used together frequently.

Code suggestion

Check the AI-generated fix before applying

Suggested change

self,

project: str = None,

domain: str = None,

name: str = None,

version: str = None,

self, entity_id: EntityIdentifier = None,

Code Review Run #941fb7

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

it was incorrectly flagged

flyte-bot · 2025-01-23T05:25:47Z

flytekit/remote/remote.py

+        upload_package_progress = Progress(TimeElapsedColumn(), TextColumn("[progress.description]{task.description}"))
+        t1 = upload_package_progress.add_task(f"Uploading package of size {content_length/1024/1024:.2f} MBs", total=1)
+
+        upload_package_progress.start_task(t1)
+
+        if is_display_progress_enabled():
+            upload_package_progress.start()


Consider consolidating progress bar initialization logic

Consider consolidating the progress bar initialization and start logic. Currently there are two separate calls - start_task() and start() with a conditional check in between, which could be simplified.

Code suggestion

Check the AI-generated fix before applying

- t1 = upload_package_progress.add_task(f"Uploading package of size {content_length/1024/1024:.2f} MBs", total=1) - upload_package_progress.start_task(t1) - if is_display_progress_enabled(): - upload_package_progress.start() + if is_display_progress_enabled(): + t1 = upload_package_progress.add_task(f"Uploading package of size {content_length/1024/1024:.2f} MBs", total=1) + upload_package_progress.start()

Code Review Run #941fb7

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

it was incorrectly flagged

flyte-bot · 2025-01-23T05:25:48Z

flytekit/tools/fast_registration.py

+        l = len(ls)
+        t = 0


Consider more descriptive variable names

Consider using more descriptive variable names instead of single letter variables l and t. Perhaps total_files and files_processed would be more meaningful.

Code suggestion

Check the AI-generated fix before applying

Suggested change

l = len(ls)

t = 0

total_files = len(ls)

files_processed = 0

Code Review Run #941fb7

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

it was incorrectly flagged

flyte-bot · 2025-01-23T05:25:49Z

flytekit/loggers.py

@@ -186,5 +187,9 @@ def get_level_from_cli_verbosity(verbosity: int) -> int:
        return logging.DEBUG


+def is_display_progress_enabled() -> bool:
+    return os.getenv(FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR, False)


Consider type conversion for env var

The os.getenv() call for FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR should specify the type for the default value as str to match environment variable type. Consider using os.getenv(FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR, 'false').lower() == 'true' for proper boolean conversion.

Code suggestion

Check the AI-generated fix before applying

Suggested change

return os.getenv(FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR, False)

return os.getenv(FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR, 'false').lower() == 'true'

Code Review Run #941fb7

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

it was incorrectly flagged

Tracks progress for package creation, upload and kickoff

59b6742

Signed-off-by: Ketan Umare <[email protected]>

kumare3 requested review from wild-endeavor, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners November 16, 2024 23:38

Mecoli1219 reviewed Nov 17, 2024

View reviewed changes

eapolinario reviewed Nov 18, 2024

View reviewed changes

kumare3 and others added 7 commits November 18, 2024 17:45

updated

a1a6a93

Signed-off-by: Ketan Umare <[email protected]>

introduce FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR to control progress

1bbcd07

Signed-off-by: Jan Fiedler <[email protected]>

update remote.py

af70397

Signed-off-by: Jan Fiedler <[email protected]>

update fast_registration.py

b07259a

Signed-off-by: Jan Fiedler <[email protected]>

ruff format

22ecb21

Signed-off-by: Jan Fiedler <[email protected]>

ruff check fix

170c504

Signed-off-by: Jan Fiedler <[email protected]>

remove show_progress attribute from remote & fast_registration

ae55489

Signed-off-by: Jan Fiedler <[email protected]>

fiedlerNr9 force-pushed the progress-tracking-pyflyte-run branch from a6cc2ed to ae55489 Compare January 23, 2025 04:26

fiedlerNr9 and others added 2 commits January 23, 2025 05:33

Merge branch 'master' into progress-tracking-pyflyte-run

2364070

make lint

5b05419

Signed-off-by: Jan Fiedler <[email protected]>

flyte-bot reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracks progress for package creation, upload and kickoff #2935

Tracks progress for package creation, upload and kickoff #2935

kumare3 commented Nov 16, 2024 •

edited by flyte-bot

Loading

codecov bot commented Nov 17, 2024 •

edited

Loading

Mecoli1219 left a comment

eapolinario Nov 18, 2024 •

edited

Loading

kumare3 Nov 19, 2024

eapolinario Nov 22, 2024 •

edited

Loading

fiedlerNr9 commented Jan 23, 2025 •

edited

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

Code Review Agent Run #941fb7

flyte-bot commented Jan 23, 2025

Changelist by Bito

flyte-bot Jan 23, 2025

flyte-bot Jan 23, 2025

flyte-bot Jan 23, 2025

flyte-bot Jan 23, 2025

flyte-bot Jan 23, 2025

-        l = len(ls)
-        t = 0
+        total_files = len(ls)
+        files_processed = 0

	return os.getenv(FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR, False)
	return os.getenv(FLYTEKIT_DISPLAY_PROGRESS_ENV_VAR, 'false').lower() == 'true'

Tracks progress for package creation, upload and kickoff #2935

Are you sure you want to change the base?

Tracks progress for package creation, upload and kickoff #2935

Conversation

kumare3 commented Nov 16, 2024 • edited by flyte-bot Loading

Summary by Bito

codecov bot commented Nov 17, 2024 • edited Loading

Codecov Report

Mecoli1219 left a comment

Choose a reason for hiding this comment

eapolinario Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

kumare3 Nov 19, 2024

Choose a reason for hiding this comment

eapolinario Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

fiedlerNr9 commented Jan 23, 2025 • edited Loading

flyte-bot commented Jan 23, 2025 • edited Loading

Code Review Agent Run #941fb7

flyte-bot commented Jan 23, 2025

Changelist by Bito

flyte-bot Jan 23, 2025

Choose a reason for hiding this comment

flyte-bot Jan 23, 2025

Choose a reason for hiding this comment

flyte-bot Jan 23, 2025

Choose a reason for hiding this comment

flyte-bot Jan 23, 2025

Choose a reason for hiding this comment

flyte-bot Jan 23, 2025

Choose a reason for hiding this comment

kumare3 commented Nov 16, 2024 •

edited by flyte-bot

Loading

codecov bot commented Nov 17, 2024 •

edited

Loading

eapolinario Nov 18, 2024 •

edited

Loading

eapolinario Nov 22, 2024 •

edited

Loading

fiedlerNr9 commented Jan 23, 2025 •

edited

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading