Why switch from relative to absolute imports? Tabular, core, common, and features all use relative imports.

Is there a general guideline you are following for favoring absolute over relative?

Nothing particular in this case, it's just that these will move into tabular anyway. I can switch to relative imports for the ones that remain after that.

PEP8 recommends absolute imports but I would vote in favor of consistency with other modules here.

This logic and variants of it can probably be put into utility methods since they are so common. Maybe as a fast follow PR. Will change a lot of 2-4 line logic into 1 line, and improve type hinting in the IDE so we can be more explicit about the type of the variable in the code.

The low_memory logic is something I implemented a very long time ago but at a certain point gave up on testing/using, since users didn't really care too much that files were being saved on disk, and it was a hassle trying to get everything working in-memory.

If time-series isn't using it, we may consider removing the low_memory logic entirely in a follow-up PR, it would make the code simpler and avoid us having to juggle multiple input/output variable types depending on the low_memory setting.

agreed. 👍

I remember wanting to refactor some of this logic as currently we have self.path and self.path_root which is a bit confusing. I might look into it closer as a follow-up.

The overall problem is that self.path doesn't contain all of the artifacts needed by the Trainer, such as those from path_utils. In an ideal world having it be self contained would be nice, kind of how Predictor is self-contained. But I would need to think about it more, because I also thought about a world where we could have multiple trainers for a single predictor, in which case it would be good for trainers to re-use certain artifacts between them.

PEP8 recommends absolute imports but I would vote in favor of consistency with other modules here.

Nit: Some methods are missing type hints.

not a nit at all and thanks for pointing it out.

a heads up, one thing that will come up in the next PR is how we handle this AbstractModel type being passed around. Because TimeSeriesTrainer will like to only work with AbstractTimeSeriesModel, however if we constrain it trivially in the function signature then pyright will complain LSP is violated. We will then have to revisit this class to use generics...

class AbstractTrainer(Generic[T]):
    def work_with_model(model: T):
        ...

class AbstractTimeSeriesTrainer(AbstractTrainer[AbstractTimeSeriesModel]):
    ...

note the syntax gets much lighter for working with generics starting from 3.11.

I was really hoping that by specializing in Python instead of C++ I will be able to avoid learning what a generic is... welp