FM-v4 branch into main #752
Conversation
Replace BalancedBatchSampler's `force_balancing` and `throw_on_error` parameters with `on_error`
Codecov ReportAttention: Patch coverage is
|
src/fairchem/core/common/utils.py
Outdated
| @@ -393,7 +402,11 @@ def create_dict_from_args(args: list, sep: str = "."): | |||
| return return_dict | |||
|
|
|||
|
|
|||
| def load_config(path: str, previous_includes: list | None = None): | |||
| def load_config( | |||
| path: str, previous_includes: list | None = None, include_paths: list | None = None | |||
There was a problem hiding this comment.
maybe comment on the diff between paths, previous_includes and include_paths?
There was a problem hiding this comment.
also optional - add test on yml with include paths if doesnt exist, can also make task for BE :)
src/fairchem/core/common/utils.py
Outdated
| @@ -999,34 +1024,70 @@ class _TrainingContext: | |||
| task_name = "s2ef" | |||
| elif trainer_name in ["energy", "equiformerv2_energy"]: | |||
| task_name = "is2re" | |||
| elif "multitask" in trainer_name.lower(): | |||
There was a problem hiding this comment.
can we get rid of this .lower() and make it exact, this makes it ambiguous on how to specify the trainer name
src/fairchem/core/common/utils.py
Outdated
| raise RuntimeError( | ||
| f"Required key missing from config: {missing_keys!s}" | ||
| ) | ||
| trainer = trainer_cls( |
There was a problem hiding this comment.
can we move these into dict and pass in by trainer_cls(**dict), add the arguments that diff manually for multitask, then we can specify the common arguments between trainer and multitask trainer once without copying
| @@ -13,7 +13,9 @@ | |||
| from torch_geometric.data import Data | |||
|
|
|||
|
|
|||
| def rename_data_object_keys(data_object: Data, key_mapping: dict[str, str]) -> Data: | |||
| def rename_data_object_keys( | |||
There was a problem hiding this comment.
comment on what this does
| @@ -571,6 +575,9 @@ def load_checkpoint( | |||
| self.step = checkpoint.get("step", 0) | |||
| self.best_val_metric = checkpoint.get("best_val_metric", None) | |||
| self.primary_metric = checkpoint.get("primary_metric", None) | |||
| self.config["cmd"]["parent"] = checkpoint["config"]["cmd"].get( | |||
There was a problem hiding this comment.
where is this parent actually used?
There was a problem hiding this comment.
I dont think its read anywhere, i believe it was intended to link fine tuning runs to their parent runs. @mshuaibii is this right?
There was a problem hiding this comment.
lets remove if its not used
There was a problem hiding this comment.
Yeah it was introduced to organize things in wandb by this entry. But you guys probably have a different solution at this point so ommitting is fine
| from contextlib import suppress | ||
|
|
||
| with suppress(ImportError): | ||
| from fairchem.experimental.foundation_models.multi_task_dataloader.transforms.data_object import * # noqa |
| @@ -232,6 +234,8 @@ def load(self) -> None: | |||
| self.load_loss() | |||
| self.load_optimizer() | |||
| self.load_extras() | |||
| if self.config["optim"].get("load_datasets_and_model_then_exit", False): | |||
There was a problem hiding this comment.
why is this needed if this is the last line of the init anyways?
Merging a long running branch called fm-v4 created to support quick changes to main for FM work