Fix trainer detection for custom Docker images with regex pattern matching

[jskswamy]

Problem

Custom DeepSpeed Docker images were losing the mpirun command and falling back to torchrun instead. The get_runtime_trainer function only used a hardcoded ALL_TRAINERS mapping, so any custom images not in this mapping would default to the PyTorch trainer configuration.

Solution

Enhanced trainer detection with regex-based pattern matching as a fallback mechanism:

1. First priority: Check existing ALL_TRAINERS mapping for exact matches
2. Second priority: Use regex patterns to detect framework from image names:
   • DeepSpeed: (?i)deepspeed (case-insensitive)
   • MLX: (?i)mlx
   • TorchTune: (?i)torchtune
   • PyTorch: (?i)(pytorch|torch) (but not torchtune)
3. Fallback: Default to PyTorch trainer if no patterns match

Key Changes

• Added _detect_trainer_from_image_patterns() function with case-insensitive regex matching
• Modified _detect_trainer() to use pattern matching as fallback
• Added copy.deepcopy() to prevent shared state issues between trainer configurations
• Comprehensive test suite with 76 test cases covering various image name formats

Testing

• ✅ Known images from ALL_TRAINERS mapping
• ✅ Custom images with various case formats (lowercase, uppercase, mixed case)
• ✅ Images with registry prefixes, ports, and complex paths
• ✅ Edge cases and fallback scenarios
• ✅ Accelerator count logic with ML policies
• ✅ State isolation between test runs

This ensures custom DeepSpeed images like my-org/deepspeed-custom:latest correctly use mpirun instead of falling back to torchrun.

This fixes the issue #29

[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.

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

[eoinfennessy]

@jskswamy thank you for this contribution! And thank you for also adding tests for get_container_devices.

It looks great to me -- I just added two small suggestions.

[eoinfennessy]

Small suggestion to replace expected_framework with None for no-match cases. I think this make the behavior of the function being tested clearer for readers.

Suggested change

	# Edge cases - no match (should fall back to default)
	("unknown-image:latest", types.TrainerFramework.TORCH),
	("", types.TrainerFramework.TORCH),
	("nginx:latest", types.TrainerFramework.TORCH),
	("ubuntu:20.04", types.TrainerFramework.TORCH),
	],
	)
	def test_trainer_detection_from_image_patterns(
	self, image_name, expected_framework
	):
	"""Test trainer detection using image pattern matching with various case scenarios."""
	trainer = utils._detect_trainer_from_image_patterns(image_name)
	if expected_framework == types.TrainerFramework.TORCH and trainer is None:
	# For unknown images, the _detect_trainer function should return default
	# but _detect_trainer_from_image_patterns returns None
	assert trainer is None
	else:
	assert trainer is not None
	assert trainer.framework.value == expected_framework.value
	# Edge cases - no match
	("unknown-image:latest", None),
	("", None),
	("nginx:latest", None),
	("ubuntu:20.04", None),
	],
	)
	def test_trainer_detection_from_image_patterns(
	self, image_name, expected_framework
	):
	"""Test trainer detection using image pattern matching with various case scenarios."""
	trainer = utils._detect_trainer_from_image_patterns(image_name)
	if expected_framework is None:
	# For unknown images _detect_trainer_from_image_patterns returns None
	assert trainer is None
	else:
	assert trainer is not None
	assert trainer.framework.value == expected_framework.value

[eoinfennessy]

This is exactly the same as the Framework enum. Can we delete this and use Framework instead?

[jskswamy]

@eoinfennessy made all the suggested changes.

[eoinfennessy]

@jskswamy LGTM! Thank you!

[google-oss-prow]

@eoinfennessy: changing LGTM is restricted to collaborators

In response to this:

@jskswamy LGTM! Thank you!

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.

[andreyvelich]

Thank you for this contribution @jskswamy 🎉

[andreyvelich]

Why do we need to add is not None here ?

[jskswamy]

1. Torch Policy Check (if trainer_container.accelerator_count is not None)

# Essential: Prevents AttributeError when accessing None.actual_instance
# Without this check: None.actual_instance would raise AttributeError
if trainer_container.accelerator_count is not None:
    if hasattr(trainer_container.accelerator_count, 'actual_instance'):
        trainer.accelerator_count = trainer_container.accelerator_count.actual_instance

2. MPI Policy Check (if trainer_container.mpi_policy is not None)

# Essential: Prevents setting accelerator_count to None when user explicitly sets it
# Without this check: trainer.accelerator_count would be overwritten to None
if trainer_container.mpi_policy is not None:
    trainer.accelerator_count = trainer_container.mpi_policy.num_procs

3. Semantic Correctness

These checks ensure that:

• User-provided values are preserved and not overwritten
• We don't attempt operations on None objects
• The logic follows "only apply changes if the field is actually set"

Code Comments Added:

I've added explanatory comments to each check to make their necessity clear for future maintainers.

[andreyvelich]

@jskswamy I just meant that those 2 lines are the same in Python, isn't ?

elif ml_policy.mpi and ml_policy.mpi.num_proc_per_node:
elif ml_policy.mpi and ml_policy.mpi.num_proc_per_node is not None:

[jskswamy]

I think it's a subtle but important distinction in Python.

The is not None Check is Necessary

The current code is correct because 0 is a valid and meaningful value for num_proc_per_node:

# Current correct implementation
elif ml_policy.mpi and ml_policy.mpi.num_proc_per_node is not None:
    trainer.accelerator_count = ml_policy.mpi.num_proc_per_node

Why Truthiness Checking Would Break CPU-Only Training

If we used truthiness checking instead:

# This would be problematic
elif ml_policy.mpi and ml_policy.mpi.num_proc_per_node:
    trainer.accelerator_count = ml_policy.mpi.num_proc_per_node

Example Scenarios:

Scenario 1: CPU-Only Training (0 accelerators)

ml_policy.mpi.num_proc_per_node = 0  # Explicitly set to CPU-only

# With truthiness check:
if ml_policy.mpi and ml_policy.mpi.num_proc_per_node:  # 0 is falsy!
    trainer.accelerator_count = ml_policy.mpi.num_proc_per_node  # ❌ Never executes

# With is not None check:
if ml_policy.mpi and ml_policy.mpi.num_proc_per_node is not None:  # 0 is not None!
    trainer.accelerator_count = ml_policy.mpi.num_proc_per_node  # ✅ Executes correctly

Scenario 2: GPU Training (4 accelerators)

ml_policy.mpi.num_proc_per_node = 4  # Explicitly set to 4 GPUs

# Both approaches work correctly:
if ml_policy.mpi and ml_policy.mpi.num_proc_per_node:  # 4 is truthy ✅
if ml_policy.mpi and ml_policy.mpi.num_proc_per_node is not None:  # 4 is not None ✅

Scenario 3: Not Set (defaults to UNKNOWN)

ml_policy.mpi.num_proc_per_node = None  # Not explicitly set

# Both approaches work correctly:
if ml_policy.mpi and ml_policy.mpi.num_proc_per_node:  # None is falsy ✅
if ml_policy.mpi and ml_policy.mpi.num_proc_per_node is not None:  # None is None ✅

The Key Distinction

The is not None check properly distinguishes between:

• "Not set" (None) → don't override accelerator count
• "Explicitly set to 0" (0) → override with 0 (CPU-only training)
• "Explicitly set to positive number" → override with that number

[andreyvelich]

But why num_proc_per_node=0 is a valid value ?
We should not allow user to set such value or consider this as None.

[andreyvelich]

@jskswamy Did you get a chance to check this comment ?

[jskswamy]

Sorry for late reply! I've addressed this, kindly check the changes now

[andreyvelich]

@tenzen-y @Electronic-Waste @astefanutti @jskswamy @eoinfennessy @franciscojavierarceo Do we see any concerns with regex approach ? It might be a good and simple method to start with, but I can imagine use cases where it wouldn't work. For example, users might have two DeepSpeed runtimes:

• One uses torchrun
• Another uses mpirun.

Perhaps in the future we can support such scenarios.

[jskswamy]

I agree, there are use cases where image name patterns alone wouldn't be sufficient.

Current Regex Approach — Pragmatic Starting Point

The regex approach implemented serves as a practical starting point that:

• ✅ Works immediately for the majority of common use cases
• ✅ Supports all official Kubeflow trainer images out of the box
• ✅ Provides sensible defaults without requiring users to specify trainer types manually
• ✅ Maintains backward compatibility with existing workflows

Future API-Based Enhancement

For advanced scenarios like your DeepSpeed example (torchrun vs mpirun variants), we can introduce explicit API controls that override the regex detection:

# Option 1: Explicit trainer specification
trainer = Trainer(
    image="custom/deepspeed-runtime",
    trainer_type=TrainerType.DEEPSPEED_MPI,  # Override regex detection
    # ... other configs
)

# Option 2: Runtime configuration
trainer = Trainer(
    image="custom/deepspeed-runtime", 
    runtime_config=DeepSpeedConfig(launcher="mpirun"),  # vs "torchrun"
    # ... other configs
)

Approach

The regex approach handles ~90% of use cases elegantly, while keeping the door open for API-based precision when needed.

Question: Which approach would you prefer to proceed with?

Option A: Keep the current regex-based detection and enhance it incrementally with API overrides when needed

Option B: Move to a more explicit API-first approach where users specify trainer types directly

Option C: Hybrid approach where regex provides defaults, but API allows explicit overrides from day one

I'm happy to implement the changes that would be most valuable for users. What are your thoughts?

[andreyvelich]

Let's keep the regex approach initially, we can have discussion how to cover more complex use-cases.
We just need to ensure we document that in our docs: https://www.kubeflow.org/docs/components/trainer/user-guides/

[andreyvelich]

Do we really need to keep framework argument given that TRAINER_CONFIGS Dict has the Framework type in the Dict key.

Suggested change

framework=Framework.TORCH,

[jskswamy]

Regarding the framework field in the Trainer class, I'd like to share my thoughts on why this field exists and why it serves a legitimate purpose:

The framework Field Has Critical Importance

After investigating the codebase, I discovered that the Trainer class and framework field were pre-existing before this PR. The field was intentionally designed to serve specific purposes:

Critical Importance for API Design

The framework field is essential for maintaining a clean, self-contained API:

1. Object Identity: A Trainer object must "know" what framework it represents without external context
2. API Completeness: When users receive a Trainer object, they can immediately determine its framework without reverse-engineering from other fields
3. Serialization: The field is crucial for JSON serialization/deserialization of trainer objects
4. Debugging & Logging: Essential for meaningful error messages and debugging information

Self-Contained Data Structure

The framework field makes Trainer objects self-contained and self-documenting:

# Example: A Trainer object "knows" what framework it represents
trainer = TRAINER_CONFIGS[Framework.DEEPSPEED]

# Self-documenting: The object tells us what it is
print(f"Using {trainer.framework} trainer with {trainer.trainer_type}")
# Output: "Using Framework.DEEPSPEED trainer with TrainerType.CUSTOM_TRAINER"

# Without the field, we'd need external context to know what framework this is
# We'd have to track which dictionary key was used to create this trainer

Breaking Changes Would Be Required

Removing the field would require:

• Modifying any code that relies on the field for framework identification
• Potentially breaking API consumers who expect this field
• Adding complex lookup logic to determine framework from other properties

Architectural Integrity

The field maintains the principle of encapsulation — Trainer object should contain all information about itself, including what framework it represents.

Why Dictionary Instead of Array?

The choice of using TRAINER_CONFIGS: Dict[Framework, Trainer] instead of an array of trainers was a performance and design optimization:

Performance Benefits

# Current efficient approach with dictionary
trainer = TRAINER_CONFIGS[Framework.DEEPSPEED]  # O(1) lookup
framework = trainer.framework  # Direct access

# Alternative inefficient approach with array
def find_trainer_by_framework(framework):
    for trainer in TRAINER_ARRAY:  # O(n) search
        if trainer.framework == framework:
            return trainer

Design Benefits

1. Fast Lookup: O(1) constant time access instead of O(n) linear search
2. Type Safety: Dictionary keys ensure we only access valid frameworks
3. Explicit Mapping: Clear relationship between framework and trainer configuration
4. Extensibility: Easy to add new frameworks without changing lookup logic

My Take

The framework field serves critical architectural purposes for API design and object encapsulation. The dictionary structure provides performance benefits, but the field itself is essential for maintaining clean, self-contained objects.

Removing the field would break the original design intent, make the API less clean and efficient, and potentially introduce breaking changes. The field was intentionally designed this way for good reasons, and I believe we should keep it to maintain the integrity of the API design.

[andreyvelich]

I agree that we should have dict to represent all Trainers where key is the Framework name and value is the Trainer object.
The question is should we also keep framework argument in the Trainer object. This is mostly used to just show users what framework this Trainer is using.

I am fine to keep it for now.

WDYT @szaher @astefanutti @Electronic-Waste ?

[andreyvelich]

I think, this could be simplified if detect_trainer_from_image_patterns just return copy.deepcopy(types.DEFAULT_TRAINER) instead of None.

Can you just keep all of the required code to extract trainer in the get_trainer_from_image() function, which accepts image_name as input?

That will make our unit tests easier to maintain.

[jskswamy]

Made necessary changes to simplify the detect_trainer_from_image_patterns function

[andreyvelich]

@jskswamy Sorry for the late reply, I meant can you just use this code snippet ?

def get_runtime_trainer(....):
....
image_name = trainer_container.image.split(":")[0]
trainer = get_trainer_from_image(image_name)


def get_trainer_from_image(image_name: str) -> types.Trainer:
    """
    Detect trainer type based on image name patterns using regex.
    This method uses pattern matching on the image name to determine
    the likely trainer type.
    Args:
        image_name: The container image name.
    Returns:
        Trainer: Trainer object if detected, otherwise the DEFAULT_TRAINER is returned.
    """
    # DeepSpeed patterns
    if re.search(r"deepspeed", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.DEEPSPEED])

    # MLX patterns
    if re.search(r"mlx", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.MLX])

    # TorchTune patterns (check before PyTorch to avoid conflicts)
    if re.search(r"torchtune", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.TORCHTUNE])

    # PyTorch patterns - require explicit "pytorch" in image name for clarity
    if re.search(r"pytorch", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.TORCH])

    return copy.deepcopy(types.DEFAULT_TRAINER)

[jskswamy]

I've simplified the function as per your suggestion, kindly check

[andreyvelich]

Sorry for the late review, I think we almost ready to merge this.
@szaher @kramaranya @briangallagher @eoinfennessy Can you take a look as well please ?

[andreyvelich]

@jskswamy Sorry for the late reply, I meant can you just use this code snippet ?

def get_runtime_trainer(....):
....
image_name = trainer_container.image.split(":")[0]
trainer = get_trainer_from_image(image_name)


def get_trainer_from_image(image_name: str) -> types.Trainer:
    """
    Detect trainer type based on image name patterns using regex.
    This method uses pattern matching on the image name to determine
    the likely trainer type.
    Args:
        image_name: The container image name.
    Returns:
        Trainer: Trainer object if detected, otherwise the DEFAULT_TRAINER is returned.
    """
    # DeepSpeed patterns
    if re.search(r"deepspeed", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.DEEPSPEED])

    # MLX patterns
    if re.search(r"mlx", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.MLX])

    # TorchTune patterns (check before PyTorch to avoid conflicts)
    if re.search(r"torchtune", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.TORCHTUNE])

    # PyTorch patterns - require explicit "pytorch" in image name for clarity
    if re.search(r"pytorch", image_name, re.IGNORECASE):
        return copy.deepcopy(types.TRAINER_CONFIGS[types.Framework.TORCH])

    return copy.deepcopy(types.DEFAULT_TRAINER)

[andreyvelich]

@jskswamy Did you get a chance to check this comment ?

[astefanutti]

I may not have the full context, so please pardon my naive question, why those metadata, mostly the framework type, could not come from the training runtime itself, in the form of an annotation or a label?

For custom images, as a platform admin / user, I would understand I need to provide some hints.

[andreyvelich]

o please pardon my naive question, why those metadata, mostly the framework type, could not come from the training runtime itself, in the form of an annotation or a label?

Previously we talked with @tenzen-y and @Electronic-Waste about introducing labels to the runtime that define framework type: https://cloud-native.slack.com/archives/C0742LDFZ4K/p1741266604716149?thread_ts=1741263570.091899&cid=C0742LDFZ4K

But we decided to not add more labels since this information can be retrieved from image and APIs.

[astefanutti]

But we decided to not add more labels since this information can be retrieved from image and APIs.

The implicit contract based on the image name might prove fragile for users and downstream projects.

I understand the regex-based heuristics could provide a nice last-resort as it stands, but making that contract explicit seems simpler and more robust. I still fail to see why it couldn't be enforced in the training runtime API, or at least the SDK would only fallback to the regex-based heuristics if that API contract is made optional.

[andreyvelich]

I understand the regex-based heuristics could provide a nice last-resort as it stands, but making that contract explicit seems simpler and more robust. I still fail to see why it couldn't be enforced in the training runtime API, or at least the SDK would only fallback to the regex-based heuristics if that API contract is made optional.

@astefanutti I think, we need to figure out why we even expose Runtime's trainer to the user.
Looking at the code it is only used to give user information about the Runtime:

sdk/python/kubeflow/trainer/types/types.py

Line 166 in fa5778b

class Trainer:

Information such as:

• whether this runtime can be used with CustomTrainer or with BuiltinTrainer. This is important data since users can incorrectly use CustomTrainer with builtin Runtimes cc @Electronic-Waste
• What ML framework users should use with the runtime (I think, this might be removed if we add get_runtime_packages() API like I showed in the KubeCon previously: https://youtu.be/Fnb1a5Kaxgo?t=556
• Entrypoint that is used while getting the TrainJob steps:

  sdk/python/kubeflow/trainer/utils/utils.py

  Line 260 in fa5778b

  trainjob_runtime.trainer.entrypoint

  and while creating the TrainJob using CustomTrainer from function:

  sdk/python/kubeflow/trainer/utils/utils.py

  Lines 340 to 341 in fa5778b

  	if runtime.trainer.entrypoint is None:
  	raise Exception(f"Runtime trainer must have an entrypoint: {runtime.trainer}")

Do we think that we can refactor some of this and remove the TRAINER_CONFIGS list ?
Thoughts @astefanutti @szaher @kramaranya @eoinfennessy ?

[astefanutti]

• whether this runtime can be used with CustomTrainer or with BuiltinTrainer. This is important data since users can incorrectly use CustomTrainer with builtin Runtimes cc @Electronic-Waste

Right, that equally applies to the BuiltinTrainer.config. For now there is only one TorchTuneConfig, but nothing guarantees it's compatible with the training runtime when there'll be more.

Do we think that we can refactor some of this and remove the TRAINER_CONFIGS list ?

For built-in trainers, it seems there is a tight coupling between the trainer and the runtime, so maybe folding things into runtime as the "source-of-truth" would be better.

[andreyvelich]

For built-in trainers, it seems there is a tight coupling between the trainer and the runtime, so maybe folding things into runtime as the "source-of-truth" would be better.

So do you mean that Runtime should tell users whether it is meant for CustomTrainer or BuiltinTrainer ?

[astefanutti]

For built-in trainers, it seems there is a tight coupling between the trainer and the runtime, so maybe folding things into runtime as the "source-of-truth" would be better.

So do you mean that Runtime should tell users whether it is meant for CustomTrainer or BuiltinTrainer ?

Yes, one way or another. How a runtime is supposed to be used in the SDK is logically defined by the runtime, that includes the type of trainer (built-in, custom) and the framework (PyTorch, JAX, TorchTune, ...).

[andreyvelich]

@astefanutti Do you think that framework information is still useful for SDK users if they can always run get_runtime_packages() API ?

[astefanutti]

@astefanutti Do you think that framework information is still useful for SDK users if they can always run get_runtime_packages() API ?

No, though it'd be needed for checking the typed configuration passed by users for built-in trainers is compatible with the training runtime?

[andreyvelich]

No, though it'd be needed for checking the typed configuration passed by users for built-in trainers is compatible with the training runtime?

This is correct, additionally we can't run the get_runtime_packages() API for BuiltinTrainer runtimes since by default it contains script for fine-tuning: https://github.com/kubeflow/trainer/blob/master/manifests/base/runtimes/torchtune/llama3_2/llama3_2_3B.yaml#L71.
We've done, so users can simple run this to fine-tune LLM:

client.train(
  runtime=Runtime(name="torchtune-llama3.2-3b")
)

Also, I don't think that users needs to know about installed packages in such runtimes, since they can only modify the config (e.g. fine-tuning parameters), but not the runtime packages.

Maybe for BuiltinTrainer runtime we should have two labels:

• trainer.kubeflow.org/trainer-type: builtin
• trainer.kubeflow.org/builtin-config: torchtune

If we don't want to introduce 2nd label, we can just tell users to rely on runtime name.

Thoughts @tenzen-y @astefanutti @Electronic-Waste @rudeigerc @szaher @kramaranya ?

[andreyvelich]

I think, we should refactor our Runtime class: https://github.com/kubeflow/sdk/blob/main/python/kubeflow/trainer/types/types.py#L176-L179

[astefanutti]

Maybe for BuiltinTrainer runtime we should have two labels:

• trainer.kubeflow.org/trainer-type: builtin
• trainer.kubeflow.org/builtin-config: torchtune

Yes, labels seem the most straightforward approach. There is already the trainer.kubeflow.org/accelerator label.
I wonder whether that'd make sense to go as far as to enforce those labels during training runtime admission?

[eoinfennessy]

Agreed that it would be better to add APIs to TrainingRuntime and ClusterTrainingRuntime to specify the framework instead of relying on image names and regex checks. Adding trainer type would also be useful.

But why use labels instead of adding framework and other fields to the runtime spec? This would allow us to use schema-based validation to ensure a valid framework is provided.

One idea for this that would use cross-field validation to ensure one and only one of customTrainerConfig or builtinTrainerConfig is provided. Fields framework and type could use enum-based validation:

spec:
  customTrainerConfig:
    framework: "torch"
  # OR
  builtinTrainerConfig:
    type: "torchtune"

Probably best to consider the exact APIs alongside work on kubeflow/trainer#2752.

[astefanutti]

But why use labels instead of adding framework and other fields to the runtime spec? This would allow us to use schema-based validation to ensure a valid framework is provided.

@eoinfennessy I agree with you it's a possible alternative. Labels are flexible and enable listing runtimes by label selectors, but we could conceptually consider these metadata as part of the spec.

[eoinfennessy]

Labels are flexible and enable listing runtimes by label selectors

Ah, I hadn't considered that. Yes, that could help improve the UX of the list_runtimes method by filtering results. e.g:

client.list_runtimes(trainerType="custom", framework="torch")

[andreyvelich]

There is already the trainer.kubeflow.org/accelerator label.

I am not sure if we should continue to maintain this label. IIRC, @tenzen-y has concerns introducing this label in the runtimes.

Labels are flexible and enable listing runtimes by label selectors,

We can also use field selector, if we introduce a new API in the runtime.

What are the pros and cons to add this property under labels or APIs ?

[astefanutti]

There is already the trainer.kubeflow.org/accelerator label.

I am not sure if we should continue to maintain this label. IIRC, @tenzen-y has concerns introducing this label in the runtimes.>

You're right, it may not be a good example "semantically".

Labels are flexible and enable listing runtimes by label selectors,

We can also use field selector, if we introduce a new API in the runtime.

I'm not sure custom fields from CRDs are indexed. It might be only few fields from core APIs.

What are the pros and cons to add this property under labels or APIs ?

I would say labels are more "free-form" and not as strictly part of the API contract compared to fields.

[andreyvelich]

I'm not sure custom fields from CRDs are indexed. It might be only few fields from core APIs.

You are right @astefanutti, here is the list of supported fields: https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/#list-of-supported-fields

You're right, it may not be a good example "semantically".

Let me remove this label for now, unless we design better way to explain users the accelerator types in the Runtime.

@Electronic-Waste @astefanutti @tenzen-y Any concerns to introduce these three labels to our runtimes for now ?

trainer.kubeflow.org/trainer-type: custom
or
trainer.kubeflow.org/trainer-type: builtin
trainer.kubeflow.org/builtin-config: torchtune

Alternatively, we can introduce framework label to the runtimes, but not sure if that is really needed, since users only need to know builtin config type to use while creating TrainJob using BuiltinTrainer.

trainer.kubeflow.org/trainer-type: custom
trainer.kubeflow.org/framework: torch
---
trainer.kubeflow.org/trainer-type: custom
trainer.kubeflow.org/framework: deepspeed
---
trainer.kubeflow.org/trainer-type: builtin
trainer.kubeflow.org/framework: torchtune

[astefanutti]

@Electronic-Waste @astefanutti @tenzen-y Any concerns to introduce these three labels to our runtimes for now ?

trainer.kubeflow.org/trainer-type: custom
or
trainer.kubeflow.org/trainer-type: builtin
trainer.kubeflow.org/builtin-config: torchtune

I think that's a good start. I only wonder whether those should be within the sdk.kubeflow.org prefix since it's metadata meant for the SDK.

Alternatively, we can introduce framework label to the runtimes, but not sure if that is really needed, since users only need to know builtin config type to use while creating TrainJob using BuiltinTrainer.

trainer.kubeflow.org/trainer-type: custom
trainer.kubeflow.org/framework: torch
---
trainer.kubeflow.org/trainer-type: custom
trainer.kubeflow.org/framework: deepspeed
---
trainer.kubeflow.org/trainer-type: builtin
trainer.kubeflow.org/framework: torchtune

Actually framework might be a different kind of metadata than those meant to be hints for the train API in the SDK.
That one would might be a simpler way to display the runtime framework rather than relying on the runtime name.

[andreyvelich]

I think that's a good start. I only wonder whether those should be within the sdk.kubeflow.org prefix since it's metadata meant for the SDK.

I think, if we keep the sdk.kubeflow.org, we need to ensure that we don't require to use the same label for other Kubeflow projects that we want to integrate into SDK.

Actually framework might be a different kind of metadata than those meant to be hints for the train API in the SDK.

@astefanutti If we establish the contract that builtin configs contain framework name in the DataClass name, the trainer.kubeflow.org/framework: torchtune is sufficient.

[astefanutti]

@astefanutti If we establish the contract that builtin configs contain framework name in the DataClass name, the trainer.kubeflow.org/framework: torchtune is sufficient.

That would be good yes. Having a mapping between framework name and DataClass name in the SDK would be perfectly acceptable I think.