config: add a simple dataclass based config system - #12
Conversation
Config objects are also validated using Pydantic Breaking changes: - train test validation split is now configured together, easier to spot mistakes - toml is supported FIXME: only 2 of the old configs have been converted
|
Thanks for the PR, I added a small comment on tomli, and have a design question: Now I see what you mean with toml and yaml support both being enabled. It makes me wonder: if we have two ML models, says one with called To answer your questions:
Yes and no; It's derived from the dataset since every dataset has different features available. But some might want to use only a subset of the features. I mainly put it it in it's own config since I didn't want to copy it between configurations as they would become very long. Would it be easy to create one canonical parser, but allow for pointing to use include type statements for other configs?
Yes, I would add it as a warning. This allows user to easily select subsets for testing the architecture etc.
Since we're still in 0.2, I would prefer to just make it breaking and move to toml altogether.
If the user only wants to use a constant learning rate and fixed amount of epochs, they could specify the options inside the learning_schemes structure directly in the root namespace. But if this introduces a lot of complexity, I think it's fine to enforce the namespace of learning_schemes with at least
It really only makes sense in the learning_schemes context. |
This kind of conditional validation can get complicated. Pydantic has a very specific order of validation. While it's possible to do, I've found it's harder to maintain. What I would prefer is, there is a different signal, say a cli argument, or maybe an attribute in the config that lets us choose the right model. I think discriminated union could work here. Here's how it would look like: In the above case, the particular array type is determined by the
Okay, I get it. I'm not happy about it, but I also can't say I have a better solution for this :-p
Including other config files can be messy IMO. When you allow that, several additional factors start playing a role, like working directory (for relative paths), precedence of values when there is an overlap, etc. What we could do is, treat this path as another type of config file with a separate "model" and validate that as a second step. We could also take into account other info, e.g. we now know the dataset path, so we could detect typos in the config if names don't match. But in that case this second "model" probably should be very simple.
I'll take this route, quite clean.
Not sure I follow, do you mean this attribute should be present when |
I guess the configs for |
I see what you mean! Do we need a better typing system then? I'm mostly thinking how we could allow for several models types, latent is already implemented and FNO might be something we want to implement in the future.
I think this works, with precendce I think it's fine to throw errors just to be safe. It's mostly that things are often used in a composable fashion. One example would be that I want to run experiments on Snellius with one common dataset, actually it would make a lot of sense to specify the dataset just once in a common config and then models separately. I'm not sure if there is a good solution for such "composable" configurations? In the end it's fine if it's all mashed together at runtime and run through just one parser...
If there is only one "learning scheme", it would be fine to put learning rate and epochs in the root namespace. But once you have more than one, it has to be put in a |
|
Do you need more input from my side to move forward? |
No need for a separate typing system. I already mocked up something using discriminated unions. Can you give a possible set of options that I can use a placeholder for a future FNO model? Based on your earlier comment, I have used something like this:
Okay, that use case makes sense. I'll think how best to do this. Basically the issue is having one config file refer to another. If instead the files are provided independently, and are combined based on a clear precedence rule, e.g. order on the command line, then it is pretty straightforward.
It's difficult to validate if certain attributes are present/absent for different cases. I'll enforce it as a list, and for the simple case there will be only a single entry. For now I think I have everything. The new conceptual work is small, I think the big part is making the existing code & tests compatible with the new config. I think I'll work on this again next week. |
This is perfect for now, I find that if you have only N=2 you are way further ahead than with N=1.
Super
Sounds good as well
Excellent! |
|
Hi @GijsVermarien, thanks for updating the dependencies. I was finding it difficult :-p What do you think of the PR otherwise? Can you review it? |
|
Hi Suvayu, great to hear it's ready for review. Could you quickly double check the original TODOs to clarify which you addressed and which you didn't yet. Cheers, Gijs |
|
Sure, this addresses:
I have started another branch on top of this ( I'll tackle the data loader after this in a separate PR. |
GijsVermarien
left a comment
There was a problem hiding this comment.
I went through the changes and it looks fine. Let's accept and see if anything pops up whilst I retrain the model on a new dataset I received recently.
There was a problem hiding this comment.
Why do we need Python<3.11 for tomli at this time?
Features
Breaking changes
spot mistakes
Questions / Remarks
input_features_fileconfig should be a config.It seems it's part of the dataset, and should be addressed by
incorporating the information in the data format.
train_test_val_splitadds up to 1.changes mentioned above, it's not 100% backwards compatible.
learning_schemesbe mandatory?time_series_fractionrequired only whenlearning_schemesarenot present? Currently it's ignored.
TODOs
latent,fno, etcSample session
(I edited both files to fix the breaking changes)