[immanuwell]

Small fix, but real.

Bug:
get_config_from_env() sets ignore_patterns=None when IGNORE_PATTERNS is unset.
That wipes the dataclass defaults for model initializers.

Why this matters:
S3().load_config() then leaves ignore_patterns empty, so S3 model downloads can pull .bin and .pt files too. Kinda sneaky, and pretty easy to hit with mixed checkpoint dirs.

Fix:
Keep the default-factory value for ignore_patterns when the env var is missing.
Added a regression test on the real S3().load_config() path.

Repro before this patch:

import os
from pkg.initializers.model.s3 import S3

os.environ["STORAGE_URI"] = "s3://models/path"
os.environ.pop("IGNORE_PATTERNS", None)

s3 = S3()
s3.load_config()
print(s3.config.ignore_patterns)

Before: None
After: ['*.msgpack', '*.h5', '*.bin', '*.pt', '*.pth']

Checks:
PYTHONPATH=$(pwd) pytest -q pkg/initializers/utils/utils_test.py pkg/initializers/model/s3_test.py pkg/initializers/model/huggingface_test.py

[github-actions]

🎉 Welcome to the Kubeflow Trainer! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
• Our team will review your PR soon! cc @kubeflow/kubeflow-trainer-team

Join the community:

• Slack: Join our #kubeflow-trainer Slack channel.
• Meetings: Attend the Kubeflow AutoML and Training Working Group bi-weekly meetings.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates environment-driven config loading so ignore_patterns defaults aren’t unintentionally overwritten when the env var isn’t set, and adjusts/extends tests accordingly.

Changes:

• Update get_config_from_env to preserve dataclass defaults for ignore_patterns when the env var is absent.
• Relax expectations in existing utils tests that previously asserted ignore_patterns=None.
• Add an S3 test to ensure default ignore_patterns remain intact after load_config().

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
pkg/initializers/utils/utils.py	Adjusts ignore_patterns env parsing to avoid overriding defaults when unset.
pkg/initializers/utils/utils_test.py	Updates test expectations to align with preserved defaults behavior.
pkg/initializers/model/s3_test.py	Adds regression test ensuring S3 default ignore_patterns are preserved.

[google-oss-prow]

[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 assign andreyvelich for approval. 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:

• OWNERS

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

[immanuwell]

@akshaychitneni @jinchihe please take a look

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.

[andreyvelich]

Sorry for the delay @immanuwell, could you rebase this PR, so we can merge this bug fix in 2.4 release?
/reopen

[google-oss-prow]

@andreyvelich: Reopened this PR.

Details

In response to this:

Sorry for the delay @immanuwell, could you rebase this PR, so we can merge this bug fix in 2.4 release?
/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[immanuwell]

@andreyvelich done, rebased onto current master, python unit and integration tests pass locally

[akshaychitneni]

/lgtm
thanks @immanuwell

[andreyvelich]

Could you add more test cases to the test_get_config_from_env as described here: https://github.com/immanuwell/trainer/blob/b2e49370d02aa93622a227251e236c23bb5a554e/AGENTS.md#python-testing-patterns

Suggested change

	def test_get_config_from_env_preserves_default_value(mock_env_vars):
	mock_env_vars(IGNORE_PATTERNS=None)
	result = utils.get_config_from_env(IgnorePatternsWithDefault)
	assert result == {}
	assert IgnorePatternsWithDefault(**result).ignore_patterns == ("*.ckpt",)
	def test_get_config_from_env_empty_ignore_patterns_clears_default(mock_env_vars):

[andreyvelich]

same suggestion

Suggested change

def test_load_config_preserves_default_ignore_patterns(mock_env_vars):