Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The docstring still references from kfp_components.components import training, but pyproject.toml (which defined the kfp_components package mapping) is deleted in this PR. These import paths are now dangling. Update the docstrings to reflect the new package structure, or remove them until the SDK integration defines the final import paths.

File renamed without changes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The "Metadata" section manually duplicates everything from metadata.yaml (tags, dependencies, owners, last-verified date). This creates a maintenance burden — any metadata.yaml change requires a synchronized README update. Consider auto-generating this section or just linking to the YAML.

File renamed without changes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

import kfp.compiler is only used inside the if __name__ == "__main__" guard. Move it inside that block to avoid loading the compiler module during normal component usage and test execution.

File renamed without changes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Two issues:

  1. Zero assertions. This calls the component but never checks outputs — not even that the output datasets exist or have rows. "Didn't crash" is not a meaningful test contract.
  2. Undeclared network dependency. This hits the real HuggingFace endpoint (dvgodoy/yoda_sentences), making it an integration test that will fail in air-gapped CI. Either mock the download or move this to a separate integration test suite with appropriate markers.

File renamed without changes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tautological test: test_component_function_exists is redundant — if the top-level import from ..component import prepare_yoda_dataset fails, the entire module fails to load and this test never runs. It's asserting something Python's import system already guarantees. Remove it.

Mock-mirror anti-pattern: test_component_with_default_parameters reproduces the component's internal call sequence step-by-step via mocks. This makes the test brittle to any refactor (e.g., combining rename_column calls) even if output is unchanged. The actual transformation logic (add_yoda_prefix) is never tested against real data — .map.assert_called_once() only checks that .map() was invoked, not that it does the right thing.

Consider testing the add_yoda_prefix function directly with a small fixture dataset instead.

File renamed without changes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file has name: invalid-dependency-semantic-versioning with fields like whatever: foo — clearly test fixture data. It shouldn't live in the production component tree under deployment/. Move it to a test_data/ or fixtures/ directory to avoid confusion.

File renamed without changes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same stale-docstring issue as library/components/__init__.py — references kfp_components.pipelines which no longer resolves to anything after pyproject.toml removal.

Also: library/ itself has no __init__.py, so it's not a Python package. import library.pipelines will fail unless namespace packages are intentionally being used (which isn't documented).

File renamed without changes.
File renamed without changes.
123 changes: 0 additions & 123 deletions pyproject.toml

This file was deleted.

Loading