Refactoring of Evaluation and adding of evaluate command

[bigximik]

✨ Description

Creates Evaluator abstraction so additional evaluators beyond Loss can be added.

Adds an evaluate command that accepts the same training config and enables evaluation on the last checkpoint.

Includes some fixes.

Example: specifying multiple LossEvaluators

training:
  evaluators:
    the_stack:
      interval: 50
      evaluator:
        type: loss
        iterations: 25
        dataset_name: the_stack
    fineweb:
      interval: 100
      evaluator:
        type: loss
        iterations: 15
        dataset_name: fineweb
data:
  datasets:
    the_stack:
      type: file
      path: path/to/validation_the_stack_dataset.yaml
    fineweb:
      type: file
      path: path/to/validation_fineweb_dataset.yaml

🔍 Type of change

Select all that apply:

• 🐛 Bug fix (non-breaking change that addresses a specific issue)
• 🚀 New feature (non-breaking change that adds functionality)
• ⚠️ Breaking change (a change that could affect existing functionality)
• 📈 Performance improvement/optimization (improves speed, memory usage, or efficiency)
• 🛠️ Code refactor (non-functional changes that improve code readability, structure, etc.)
• 📦 Dependency bump (updates dependencies, including Dockerfile or package changes)
• 📝 Documentation change (updates documentation, including new content or typo fixes)
• 🔧 Infrastructure/Build change (affects build process, CI/CD, or dependencies)

📝 Changes

✅ Checklist

Make sure the following tasks are completed before submitting the PR:

General

• 📜 I have read and followed the contributing guidelines.
• 🏷️ I am using a clear and descriptive PR title that summarizes the key change or feature introduced.
• 🎉 The functionality is complete, and I have tested the changes.
• 📝 I have updated the documentation if needed.
• ⚠️ The change does not introduce any new issues (e.g., runtime warnings, type checker errors, linting problems, unhandled edge cases).
• 🧩 I have commented my code, especially in hard-to-understand areas.

Testing

• 🧪 I have added or updated tests to cover my changes.
• ✔️ New and existing tests pass locally with my changes.
• 🚦 I have tested these changes on GPUs and verified training stability.

[jlamypoirier]

It should still inherit from IntervalConfig so it's simpler and we don't have the redundant run_interval.

[bigximik]

I prefer composition over multiple inheritance here, as IntervalConfig is a property of the evaluator wrapper (which TrainingEvaluatorConfig is), and not of the new entity derived from EvaluatorConfigBase. In my opinion, this makes the code much more readable.

However, multiple inheritance is already used in many places. So if you still prefer that approach after my explanation, I’m happy to refactor accordingly.

[jlamypoirier]

Multiple inheritance is only needed because of the EvaluatorConfigBase mixin, which is arguably not even necessary. I'd rather prioritize better usage (configs) over marginally simpler code.

[bigximik]

OK, I will use multiple inheritance but will retain EvaluatorConfigBase so I don't need to rewrite EvaluatorRunner then we will create a separate config for evaluate command in addition for it of accepting training config. We need to discuss what it should look like, so I’ve created an issue for it #285.

[jlamypoirier]

Seems unnecessary, we can just call evaluator.get_evaluator()

[bigximik]

I explained in details #222 (comment) and #222 (comment) but shortly it is to maintain proper encapsulation

[jlamypoirier]

There is no encapsulation needed though, TrainingEvaluatorConfig is a fixed class with an evaluator :EvaluatorConfig field, which dis dynamic but has a well-defined get_evaluator method.

Encapsulation would be needed if we allowed for a more generic scenario where evaluator.get_evaluator doesn't exist or has a different signature, ex. if we allowed for a more generic evaluator or a generalized TrainingEvaluatorConfig that doesn't have an evaluator. I don't really see this happening anytime soon...

[bigximik]

Another thing I can't really work around here is that I need to return TrainingEvaluator and not evaluator.get_evaluator(). This is because TrainingEvaluator is responsible for handling whether evaluators should or should not run during training. Neither the concrete evaluators nor the EvaluatorRunner are aware of this—they simply execute.

[jlamypoirier]

I don't think that's necessary

[bigximik]

I explained in details #222 (comment) and #222 (comment) but shortly it is to maintain proper encapsulation

[bigximik]

I was also thinking we should move the dataset definitions once we have evaluators, feel free to do it here or in a follow-up PR.

I will move it in the #282, so we can close this faster.

[jlamypoirier]

I don't think this test is worth it, it's kind of trivial.