Skip to content

docs: improve documentation for utils and pipeline modules#704

Open
Kaibalya-Mohanty wants to merge 9 commits into
kubeflow:mainfrom
Kaibalya-Mohanty:main
Open

docs: improve documentation for utils and pipeline modules#704
Kaibalya-Mohanty wants to merge 9 commits into
kubeflow:mainfrom
Kaibalya-Mohanty:main

Conversation

@Kaibalya-Mohanty

@Kaibalya-Mohanty Kaibalya-Mohanty commented Mar 17, 2026

Copy link
Copy Markdown

Documentation Improvements

  • Improved docstrings for several utility functions:

    • random_string
    • abs_working_dir
    • shorten_long_string
    • dedent
    • remove_ansi_color_sequences
    • Added module-level documentation explaining pipeline architecture and execution flow:
    • Enhanced add_step method docstring
    • Added clearer explanations for pipeline structure and usage

These changes aim to improve clarity and onboarding for new contributors.

Looking forward to feedback!

Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
@google-oss-prow google-oss-prow Bot added size/L and removed size/M labels Mar 19, 2026
@Kaibalya-Mohanty Kaibalya-Mohanty changed the title Improved docstrings for several utility functions to enhance clarity and ease onboarding for new contributors. docs: improve documentation for utils and pipeline modules Mar 19, 2026
Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
@ada333

ada333 commented Mar 23, 2026

Copy link
Copy Markdown
Collaborator

@Kaibalya-Mohanty Please do only DocString changes in this PR.

@jesuino jesuino left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @Kaibalya-Mohanty

Thanks for your first contribution!

The documentation changes seems good and they are welcome! I notice that you possibly accidentally modified a function code. Would you please confirm if it was your goal to modify that or was it modified accidentally?

Thanks!

Comment thread kale/common/utils.py Outdated
"""Remove ANSI color sequences from text."""
ansi_color_escape = re.compile(r"\x1B\[[0-9;]*m")
return ansi_color_escape.sub("", text)
def remove_ansi_sequences(text: str) -> str:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this change was expected. The function was renamed, but the reference to it in jputils.py was not updated. I would also not change the function implementation because the goal of your PR was to improve the documentation.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review. The function rename and implementation changes were unintentional. I have reverted the code changes and kept only the documentation improvements.

Reverted unintended function renaming and implementation changes.
Kept only the docstring improvements for remove_ansi_color_sequences.

Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>

@ederign ederign left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kaibalya-Mohanty you are still making some code changes, can you please take a look? We will soon have more issues as well, check our issues backlog! Maybe you can find something more fun to work with!

@ederign

ederign commented Jun 30, 2026

Copy link
Copy Markdown
Member

Hi @Bagumeow, thanks for the contribution!

The CI failures are caused by this PR.

Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
@google-oss-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ederign. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
Reverted broader ANSI regex, removed isinstance check, removed early-return guard in shorten_long_string, and removed added log.debug call in rm_r.

Signed-off-by: KAIBALYA MOHANTY <168870673+Kaibalya-Mohanty@users.noreply.github.com>
@Kaibalya-Mohanty

Copy link
Copy Markdown
Author

Hi, I've addressed both issues raised in the review:

  1. Fixed the indentation errors in shorten_long_string and dedent
    docstrings that were causing CI failures.

  2. Separated the behavioral changes into a new PR fix: improve ANSI regex, add short-string guard, add debug logging in rm_r #864 to keep
    this PR docs-only as suggested.

Please let me know if there's anything else to address. Thanks!

@ederign

ederign commented Jul 1, 2026

Copy link
Copy Markdown
Member

@ada333

@ada333 ada333 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@google-oss-prow google-oss-prow Bot added the lgtm label Jul 1, 2026

@ederign ederign left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kaibalya-Mohanty those doc changes don't bring much value and we are in the third round of review and you still keep the behaviour changes.

Comment thread kale/common/utils.py
# the exception handler handle it (i.e., check ignore_missing etc.)
raise OSError(errno.ENOENT, "No such file or directory", path)
except OSError as e:
log.debug("Error while removing path: %s", path)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still behaviour change.

Comment thread kale/common/utils.py
Text with ANSI escape sequences removed.
"""

ansi_escape = re.compile(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same.

Comment thread kale/common/utils.py
str_input = str(obj)
return str_input[:chars] + " ..... " + str_input[len(str_input) - chars :]

return str_input[:chars] + " ..... " + str_input[-chars:]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are NOT equivalent when chars=0:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants