-
Notifications
You must be signed in to change notification settings - Fork 333
[Feat] Enable registered entity summary output and quiet flag in pyflyte register #3028
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
base: master
Are you sure you want to change the base?
[Feat] Enable registered entity summary output and quiet flag in pyflyte register #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
|
47d8b92
to
dda078c
Compare
Code Review Agent Run Status
|
ef2743a
to
0395846
Compare
Signed-off-by: paullongtan <[email protected]>
Signed-off-by: paullongtan <[email protected]>
Signed-off-by: paullongtan <[email protected]>
0395846
to
9bba92f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice first PR! I think you can add more unit tests.
- yaml
- testing registration without specifying
--summary-dir
flytekit/tools/repo.py
Outdated
else: | ||
summary_dir = os.getcwd() | ||
|
||
summary_file = f"registration_summary.{summary_format}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! The summary will be overwritten if the registration is rerun.
flytekit/tools/repo.py
Outdated
if not skip_errors: | ||
raise e | ||
print_registration_status(og_id, success=False) | ||
result["status"] = "failed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it fails, what will the values of other keys be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
…mmary-dir testings to unit test Signed-off-by: paullongtan <[email protected]>
2bbe0d1
to
db9f744
Compare
Code Review Agent Run #a2cf6aActionable Suggestions - 0Review Details
|
Thanks! |
…ary to output to cli Signed-off-by: paullongtan <[email protected]>
["register", "--summary-format", "yaml", "core6"] | ||
) | ||
assert result.exit_code == 0 | ||
summary_data = yaml.safe_load(result.output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Signed-off-by: paullongtan <[email protected]>
a9564f5
to
8a82a78
Compare
Code Review Agent Run #93cfadActionable Suggestions - 1
Review Details
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Code Review Agent Run #7297c4Actionable Suggestions - 2
Review Details
|
""" | ||
|
||
_original_secho = click.secho | ||
_original_log_level = logger.level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3028 +/- ##
===========================================
+ Coverage 51.81% 83.58% +31.77%
===========================================
Files 202 3 -199
Lines 21469 195 -21274
Branches 2766 0 -2766
===========================================
- Hits 11125 163 -10962
+ Misses 9735 32 -9703
+ Partials 609 0 -609 ☔ View full report in Codecov by Sentry. |
Signed-off-by: paullongtan <[email protected]>
New updates:
Note:
|
Code Review Agent Run #24c91aActionable Suggestions - 0Review Details
|
Signed-off-by: paullongtan <[email protected]>
Signed-off-by: paullongtan <[email protected]>
Code Review Agent Run #d1c6c9Actionable Suggestions - 0Additional Suggestions - 1
Review Details
|
…_output_format Signed-off-by: paullongtan <[email protected]>
Code Review Agent Run Status
|
…_output_format Signed-off-by: paullongtan <[email protected]>
Signed-off-by: paullongtan <[email protected]>
3d93794
to
5fcf464
Compare
Code Review Agent Run Status
|
e60c439
to
aad07e8
Compare
Tracking issue
Closes flyteorg/flyte#3919
Why are the changes needed?
Currently,
pyflyte register
does not have an option to output registered entities to a file. This PR creates such functionality.What changes were proposed in this pull request?
Pyflyte register
--quiet
to mute all outputs to CLI--summary-format
to output registration summary to the CLI, uses json as default format and supports both yaml and json output formatHow was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This pull request significantly enhances the `pyflyte register` command by adding options for outputting registered entities in JSON and YAML formats, along with a `--quiet` flag to suppress console output. It also introduces new agents for improved interactions and comprehensive unit tests to ensure the reliability of these features.