If you want, you can put from __future__ import annotations at the top of a file and can then use tuple[bool ,bool]

Nice! Updated

Probably not critical but it seems this call to trainer.leaderboard() is not the cheapest, given it does a lot of dict parsing, dag processing and sorting.

https://github.com/Innixma/autogluon/blob/7c407de63254ff7f8a09c63be1adb495bd392229/core/src/autogluon/core/trainer/abstract_trainer.py#L3151

Alternatives:

• You only seem to be using "score_val", you could get it directly: https://github.com/Innixma/autogluon/blob/7c407de63254ff7f8a09c63be1adb495bd392229/core/src/autogluon/core/trainer/abstract_trainer.py#L3169
• I can't tell what's in the dict from just looking at the code but it seems like the best model can also be extracted directly from this:
  https://github.com/Innixma/autogluon/blob/7c407de63254ff7f8a09c63be1adb495bd392229/core/src/autogluon/core/trainer/abstract_trainer.py#L3178-L3187

model, score = max(trainer.get_model_attributes("val_score").items(), key=lambda t: t[1]) # t = (model_name, score) 

Good call! I actually needed to revamp this logic to make it work in cases where the user specified infer_limit to ensure that the model returned satisfies the infer_limit.

Lowercase callable is actually not a type, it's a function. You need typing.Callable.

(...oh how I wish it was though, like list and dict are)

Good catch! I updated the PR and I created a GitHub issue to track replacement throughout the project: #4349

Likewise callable

ditto

Probably not worth the complexity if this is the only usage but the _fit_setup() and _fit_cleanup() can be removed in favor of a @contextmanager, i.e. with _fitting_contenxt(): , it can make the intent a bit clearer and there's one less completion your editor gives you

from contextlib import contextmanager

class WhateverThisClassIs:

    @contextmanager
    def _fitting_context(self) -> Iterator[None]:
        # Previously `_fit_start()`
        self._time_train_start = time.time()
        self._time_train_start_last = self._time_train_start
        self._time_limit = time_limit
        
        # Might be able to just lift logic here and remove one
        # more method
        self.reset_callbacks() 
        
        if callbacks is not None:
            assert isinstance(callbacks, list), f"`callbacks` must be a list. Found invalid type: `{type(callbacks)}`."
        else:
            callbacks = []
        self.callbacks = callbacks
        
        yield
        
        # Previously `_fit_cleanup()`
        self._time_limit = None
        self._time_train_start = None
                
        # Likewise with lifting logic
        self.reset_callbacks()
        
        

This is an interesting idea. I think for now I will keep it as is, but this could be something worth adopting in future. I've made essentially zero use of yield in AutoGluon so far, mostly due to my unfamiliarity.

Recently learnt that time.time() is not the best for measuring durations. Most systems are fine and it's rarely ever leading to a bug until it is one.

https://docs.python.org/3/library/time.html#time.monotonic

Nice find! #4342

Worth considering if you want some overwrite ability for this, i.e. for testing.

Interesting idea, will probably keep as is for now and add overwrite logic if it becomes useful.

If there's no side effect to care about with the .after_fit(), i.e. you don't care about callbacks state after this call, then you could break early. Might not be important if callbacks are super cheap.

    def _callbacks_after_fit(
        self,
        *,
        model_names: List[str],
        stack_name: str,
        level: int,
    ):
        for callback in self.callbacks:
            callback_early_stop = callback.after_fit(
                self,
                model_names=model_names,
                logger=logger,
                stack_name=stack_name,
                level=level,
            )
            if callback_early_stop:
                self._callback_early_stop = True
                break

Or if you're feeling functional ;)

    def _callbacks_after_fit(
        self,
        *,
        model_names: List[str],
        stack_name: str,
        level: int,
    ):
        should_stop_itr = (
            callback.after_fit(
                self,
                model_names=model_names,
                logger=logger,
                stack_name=stack_name,
                level=level,
            )
            for callback in self.callbacks
        )
        self._callback_early_stop = any(should_stop_itr)

My thought process is that some callbacks could simply be logging related, and I wouldn't want a callback that early stops to prevent a logging callback from doing its logging.

The other thing is technically the later callbacks can check if trainer._callback_early_stop == True and skip their logic.

This is a good idea though, and I thought about it too while implementing. I've added the following self.skip_if_trainer_stopped parameter to callbacks. When true, the callback logic will be skipped when trainer._callback_early_stop == True. This should give the best of both worlds.

Sounds good! If you want to make it clearer to users, you could make a shallow class LoggingCallback, or conversely, EarlyStoppingCallback which just internally sets this flag, i.e. user never sets it. I use the pattern a good bit where the flag is actually set as a class variables, e.g. LoggingCallback sets skip = False
And EarlyStoppingCallback with skip = True

Gives you the freedom to later change this to a dict or things, or whatever, as long as LoggingCallback is never skipped and EarlyStoppimhCallback is. Also one less parameter for someone to think about

That is a great idea, updated the code, I think this is a good practice I should adopt more often to separate user-level args with developer level args (aka those that are only relevant to specify when subclassing)

Should this be a typing.Literal?

in this case no because stack_name can be any valid string.

Nit: Should we use admonitions to highlight experimental features?

:::{warning}
This is an experimental feature and may change in the future releases without warning.
:::

Nice! Added

Does it make sense to verify that callbacks are of type AbstractCallback?

Good idea! Added

I'm not very familiar with the rest of the AbstractTrainer code, so this might be a bad suggestion, but would it be feasible to communicate the early stopping / interruption without altering the state of the trainer? I can imagine that in some scenarios such as distributed training it will be really tricky to reason about the state of this variable.

Tough to say, but I think I would prefer to edit the trainer state, as the intention is to not pass the trainer object to worker threads, and therefore it remains a singular source of truth. We can change it later if we find this has limitations.

qq: could you explain this check ?

Pseudocode breakdown of the check:

if (user specified infer limit) and (any model exists):
    select the best valid model based on validation score that satisfies the infer limit
elif (trainer has not specified which model is best) and (any model exists):
    select the best valid model based on validation score

The added infer_limit check ensures that we always return a model that satisfies the infer_limit constraints. Sometimes, a non-ensemble model can get a better score while satisfying infer limit than an ensemble model, and by default only ensemble models are set to self.model_best prior to this call. This call ensures that the best model will be picked, even if it isn't an ensemble.