feat: add options to output registered entity summary #3028
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Code Review Agent Run Status
|
paullongtan
force-pushed
the
feat/add_register_output_format
branch
from
47d8b92 to
dda078c
Compare
Code Review Agent Run Status
|
paullongtan
force-pushed
the
feat/add_register_output_format
branch
from
ef2743a to
0395846
Compare
Signed-off-by: paullongtan <paullongtan@gmail.com>
Signed-off-by: paullongtan <paullongtan@gmail.com>
Signed-off-by: paullongtan <paullongtan@gmail.com>
paullongtan
force-pushed
the
feat/add_register_output_format
branch
from
0395846 to
9bba92f
Compare
paullongtan
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
flytekit/tools/repo.py
Outdated
| else: | ||
| summary_dir = os.getcwd() | ||
|
|
||
| summary_file = f"registration_summary.{summary_format}" |
There was a problem hiding this comment.
Does this mean that the other registration may overwrite the summary? I think we could name the file with other unique names (ie. tmp file, py file name, wf name, version number)
There was a problem hiding this comment.
Yes! The summary will be overwritten if the registration is rerun.
flytekit/tools/repo.py
Outdated
|
|
||
| except Exception as e: | ||
| if not skip_errors: | ||
| raise e | ||
| print_registration_status(og_id, success=False) | ||
| result["status"] = "failed" |
There was a problem hiding this comment.
If it fails, what will the values of other keys be?
There was a problem hiding this comment.
The values of the other keys(id, type, version) are pre-computed before registration. Thus, the values will not be empty even if the registration fails.
The values are from:
result = {
"id": og_id.name,
"type": og_id.resource_type_name(),
"version": og_id.version,
"status": "skipped", # default status
}
where og_id is the id of the entity's template / entity itself.
Code Review Agent Run #9a3edbActionable Suggestions - 3
Additional Suggestions - 1
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
flytekit/tools/repo.py
Outdated
| result = { | ||
| "id": og_id.name, | ||
| "type": og_id.resource_type_name(), | ||
| "version": og_id.version, | ||
| "status": "skipped", # default status | ||
| } |
There was a problem hiding this comment.
Consider adding more detailed status information in the result dictionary. The current status field only captures high-level states ('skipped', 'success', 'failed'). Additional fields like error_message and timestamp could provide more context for debugging and monitoring.
Code suggestion
Check the AI-generated fix before applying
| result = { | |
| "id": og_id.name, | |
| "type": og_id.resource_type_name(), | |
| "version": og_id.version, | |
| "status": "skipped", # default status | |
| } | |
| result = { | |
| "id": og_id.name, | |
| "type": og_id.resource_type_name(), | |
| "version": og_id.version, | |
| "status": "skipped", # default status | |
| "timestamp": datetime.datetime.now().isoformat(), | |
| "error_message": "", | |
| "details": {} | |
| } |
Code Review Run #9a3edb
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
flytekit/tools/repo.py
Outdated
| supported_format = ["json", "yaml"] | ||
| if summary_format not in supported_format: | ||
| raise ValueError(f"Unsupported file format: {summary_format}") |
There was a problem hiding this comment.
Consider using a set for supported_format instead of a list since we're only checking membership. This would provide O(1) lookup instead of O(n). Could be defined as supported_format = {'json', 'yaml'}.
Code suggestion
Check the AI-generated fix before applying
| supported_format = ["json", "yaml"] | |
| if summary_format not in supported_format: | |
| raise ValueError(f"Unsupported file format: {summary_format}") | |
| if summary_format not in {"json", "yaml"}: | |
| raise ValueError(f"Unsupported file format: {summary_format}") |
Code Review Run #9a3edb
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| "--summary-format", | ||
| "-f", | ||
| required=False, | ||
| type=click.Choice(["json", "yaml"], case_sensitive=False), | ||
| default=None, | ||
| help="Set output format for registration summary. Lists registered workflows, tasks, and launch plans. 'json' and 'yaml' supported.", | ||
| ) | ||
| @click.option( | ||
| "--summary-dir", | ||
| required=False, | ||
| type=click.Path(dir_okay=True, file_okay=False, writable=True, resolve_path=True), | ||
| default=None, | ||
| help="Directory to save registration summary. Uses current working directory if not specified.", | ||
| ) |
There was a problem hiding this comment.
Consider adding validation for summary-format and summary-dir options. When summary-format is specified but summary-dir is not, the summary may not be saved correctly. Consider making summary-dir required when summary-format is provided.
Code suggestion
Check the AI-generated fix before applying
| "--summary-format", | |
| "-f", | |
| required=False, | |
| type=click.Choice(["json", "yaml"], case_sensitive=False), | |
| default=None, | |
| help="Set output format for registration summary. Lists registered workflows, tasks, and launch plans. 'json' and 'yaml' supported.", | |
| ) | |
| @click.option( | |
| "--summary-dir", | |
| required=False, | |
| type=click.Path(dir_okay=True, file_okay=False, writable=True, resolve_path=True), | |
| default=None, | |
| help="Directory to save registration summary. Uses current working directory if not specified.", | |
| ) | |
| "--summary-format", | |
| "-f", | |
| required=False, | |
| type=click.Choice(["json", "yaml"], case_sensitive=False), | |
| default=None, | |
| help="Set output format for registration summary. Lists registered workflows, tasks, and launch plans. 'json' and 'yaml' supported.", | |
| callback=validate_summary_options, | |
| ) | |
| "--summary-dir", | |
| required=False, | |
| type=click.Path(dir_okay=True, file_okay=False, writable=True, resolve_path=True), | |
| default=None, | |
| help="Directory to save registration summary. Uses current working directory if not specified.", | |
| callback=validate_summary_options, | |
| ) |
Code Review Run #9a3edb
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: paullongtan <paullongtan@gmail.com>
…mmary-dir testings to unit test Signed-off-by: paullongtan <paullongtan@gmail.com>
paullongtan
force-pushed
the
feat/add_register_output_format
branch
from
2bbe0d1 to
db9f744
Compare
Code Review Agent Run #a2cf6aActionable Suggestions - 0Review Details
AI Code Review powered by |
Thanks! |
…ary to output to cli Signed-off-by: paullongtan <paullongtan@gmail.com>
Code Review Agent Run #5721dcActionable Suggestions - 3
Review Details
AI Code Review powered by |
flytekit/tools/repo.py
Outdated
| with temporary_secho(): | ||
| if summary_format == "json": | ||
| click.secho(json.dumps(all_results, indent=2)) | ||
| elif summary_format == "yaml": | ||
| click.secho(yaml.dump(all_results)) |
There was a problem hiding this comment.
Consider using a context manager for temporarily restoring click.secho instead of directly manipulating it. The current approach with temporary_secho() could lead to inconsistent state if an exception occurs. A similar issue was also found in flytekit/clis/sdk_in_container/register.py (line 219-220).
Code suggestion
Check the AI-generated fix before applying
@@ -246,11 +246,12 @@
-def temporary_secho():
+@contextmanager
+def temporary_secho():
current_secho = click.secho
try:
click.secho = original_secho
yield
finally:
click.secho = current_secho
Code Review Run #5721dc
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| ["register", "--summary-format", "json", "core5"] | ||
| ) | ||
| assert result.exit_code == 0 | ||
| summary_data = json.loads(result.output) |
There was a problem hiding this comment.
Consider validating the result.output before parsing it as JSON to handle potential invalid JSON gracefully.
Code suggestion
Check the AI-generated fix before applying
| summary_data = json.loads(result.output) | |
| try: | |
| summary_data = json.loads(result.output) | |
| except json.JSONDecodeError as e: | |
| pytest.fail(f"Failed to parse registration summary JSON: {e}") | |
| except Exception as e: | |
| pytest.fail(f"Unexpected error while parsing registration summary: {e}") |
Code Review Run #5721dc
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| ["register", "--summary-format", "yaml", "core6"] | ||
| ) | ||
| assert result.exit_code == 0 | ||
| summary_data = yaml.safe_load(result.output) |
There was a problem hiding this comment.
Consider adding error handling when parsing YAML from result.output. The yaml.safe_load() could raise yaml.YAMLError if the output is not valid YAML.
Code suggestion
Check the AI-generated fix before applying
| summary_data = yaml.safe_load(result.output) | |
| try: | |
| summary_data = yaml.safe_load(result.output) | |
| except yaml.YAMLError as e: | |
| pytest.fail(f"Failed to parse YAML output: {e}") | |
Code Review Run #5721dc
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: paullongtan <paullongtan@gmail.com>
Signed-off-by: paullongtan <paullongtan@gmail.com>
paullongtan
force-pushed
the
feat/add_register_output_format
branch
from
a9564f5 to
8a82a78
Compare
Code Review Agent Run #93cfadActionable Suggestions - 1
Review Details
AI Code Review powered by |
| raise e | ||
| if not quiet: | ||
| print_registration_status(og_id, success=False) | ||
| result["status"] = "failed" | ||
| else: | ||
| if not quiet: | ||
| print_registration_status(og_id, dry_run=True) | ||
| except RegistrationSkipped: |
There was a problem hiding this comment.
Consider consolidating the error handling logic. Currently, there are two separate error handling paths - one for registration failure and one for RegistrationSkipped. Both paths perform similar actions (printing status and setting result status). This could be simplified to reduce code duplication.
Code suggestion
Check the AI-generated fix before applying
@@ -394,8 +394,12 @@
- print_registration_status(og_id, success=False)
- result["status"] = "failed"
- else:
- print_registration_status(og_id, dry_run=True)
- except RegistrationSkipped:
- if not quiet:
- print_registration_status(og_id, success=False)
- result["status"] = "skipped"
+ _handle_registration_error(og_id, "failed", quiet)
+ else:
+ print_registration_status(og_id, dry_run=True)
+ except RegistrationSkipped:
+ _handle_registration_error(og_id, "skipped", quiet)
+
+def _handle_registration_error(og_id, status, quiet):
+ if not quiet:
+ print_registration_status(og_id, success=False)
+ result["status"] = status
Code Review Run #93cfad
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: paullongtan <paullongtan@gmail.com>
Code Review Agent Run #7297c4Actionable Suggestions - 2
Review Details
AI Code Review powered by |
| @@ -33,6 +33,9 @@ | |||
| the root of your project, it finds the first folder that does not have a ``__init__.py`` file. | |||
| """ | |||
|
|
|||
| _original_secho = click.secho | |||
| _original_log_level = logger.level | |||
There was a problem hiding this comment.
Consider using a context manager to handle logger level changes instead of storing the level in a global variable. This would ensure proper cleanup and avoid potential side effects. A similar issue was also found in flytekit/tools/repo.py (line 290).
Code suggestion
Check the AI-generated fix before applying
| _original_log_level = logger.level | |
| class LogLevelManager: | |
| def __init__(self): | |
| self.original_level = logger.level | |
| def __enter__(self): | |
| return self | |
| def __exit__(self, exc_type, exc_val, exc_tb): | |
| logger.level = self.original_level |
Code Review Run #7297c4
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| out = subprocess.run(["git", "init"], capture_output=True) | ||
| assert out.returncode == 0 | ||
| os.makedirs("core6", exist_ok=True) | ||
| with open(os.path.join("core6", "sample.py"), "w") as f: | ||
| f.write(sample_file_contents) | ||
| f.close() | ||
|
|
||
| result = runner.invoke( | ||
| pyflyte.main, | ||
| ["register", "--summary-format", "yaml", "core6"] | ||
| ) | ||
| assert result.exit_code == 0 | ||
| try: | ||
| summary_data = yaml.safe_load(result.output) | ||
| except yaml.YAMLError as e: | ||
| pytest.fail(f"Failed to parse YAML output: {e}") | ||
| assert isinstance(summary_data, list) | ||
| assert len(summary_data) > 0 | ||
| for entry in summary_data: | ||
| assert "id" in entry | ||
| assert "type" in entry | ||
| assert "version" in entry | ||
| assert "status" in entry | ||
|
|
||
| shutil.rmtree("core6") |
There was a problem hiding this comment.
Consider moving the shutil.rmtree("core6") cleanup to a finally block to ensure cleanup happens even if assertions fail.
Code suggestion
Check the AI-generated fix before applying
| out = subprocess.run(["git", "init"], capture_output=True) | |
| assert out.returncode == 0 | |
| os.makedirs("core6", exist_ok=True) | |
| with open(os.path.join("core6", "sample.py"), "w") as f: | |
| f.write(sample_file_contents) | |
| f.close() | |
| result = runner.invoke( | |
| pyflyte.main, | |
| ["register", "--summary-format", "yaml", "core6"] | |
| ) | |
| assert result.exit_code == 0 | |
| try: | |
| summary_data = yaml.safe_load(result.output) | |
| except yaml.YAMLError as e: | |
| pytest.fail(f"Failed to parse YAML output: {e}") | |
| assert isinstance(summary_data, list) | |
| assert len(summary_data) > 0 | |
| for entry in summary_data: | |
| assert "id" in entry | |
| assert "type" in entry | |
| assert "version" in entry | |
| assert "status" in entry | |
| shutil.rmtree("core6") | |
| out = subprocess.run(["git", "init"], capture_output=True) | |
| assert out.returncode == 0 | |
| os.makedirs("core6", exist_ok=True) | |
| with open(os.path.join("core6", "sample.py"), "w") as f: | |
| f.write(sample_file_contents) | |
| f.close() | |
| try: | |
| result = runner.invoke( | |
| pyflyte.main, | |
| ["register", "--summary-format", "yaml", "core6"] | |
| ) | |
| assert result.exit_code == 0 | |
| try: | |
| summary_data = yaml.safe_load(result.output) | |
| except yaml.YAMLError as e: | |
| pytest.fail(f"Failed to parse YAML output: {e}") | |
| assert isinstance(summary_data, list) | |
| assert len(summary_data) > 0 | |
| for entry in summary_data: | |
| assert "id" in entry | |
| assert "type" in entry | |
| assert "version" in entry | |
| assert "status" in entry | |
| finally: | |
| shutil.rmtree("core6") |
Code Review Run #7297c4
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
…put format Signed-off-by: paullongtan <paullongtan@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3028 +/- ##
==========================================
- Coverage 78.19% 77.25% -0.94%
==========================================
Files 201 212 +11
Lines 21274 22062 +788
Branches 2733 2753 +20
==========================================
+ Hits 16635 17045 +410
- Misses 3843 4257 +414
+ Partials 796 760 -36 ☔ View full report in Codecov by Sentry. |
Signed-off-by: paullongtan <paullongtan@gmail.com>
|
New updates:
Note:
|
Code Review Agent Run #24c91aActionable Suggestions - 0Review Details
AI Code Review powered by |
Tracking issue
Closes flyteorg/flyte#3919
Why are the changes needed?
Currently,
pyflyte registerdoes not have an option to output registered entities to a file. This PR creates such functionality.What changes were proposed in this pull request?
--summary-formatand--summary-dirto pyflyte register inflytekit/flytekit/clis/sdk_in_container/register.py.--summary-format: Sets output format for registration summary. Lists registered workflows, tasks, and launch plans. 'json' and 'yaml' supported.--summary-dir: Directory to save registration summary. Uses current working directory if not specified.flytekit/flytekit/tools/repo.py. Results are then saved to designated directory in selected file format.register.py.How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
Enhanced pyflyte register command with JSON/YAML format support and improved console output capabilities. Implemented comprehensive logging control mechanisms including quiet mode and SuppressEcho context manager. Added support for multiple output formats while removing unnecessary logging statements. Test suite updated and refactored for better maintainability while preserving coverage.Unit tests added: True
Estimated effort to review (1-5, lower is better): 3