Skip to content

Conversation

@IlyasMoutawwakil
Copy link
Member

@IlyasMoutawwakil IlyasMoutawwakil commented Aug 24, 2023

This refactoring's purpose is to regain separation of concerns and removing patches.
This will allow for better unittests ; testing trackers separately from backends, and generators separately from benchmarks.
Another reason for this refactoring is to standardize the interaction between BackendConfig<->Backend, BenchmarkConfig<->Benchmark, Benchmark<->Backend and interactions within a backend (optimization, quantization, etc.)
Some key points:

  • many of the arguments that end with config are now optional empty dictionaries since their usage is generally related to another control argument. With this, hydra_config.yaml will not contain things that are not used by the benchmark script. For example, ort optimization/quantization configs, torch.compile's config, etc.
  • Each backend being dependent on many third party packages, it's always better to keep standard library imports separated from third party imports and from local repo imports.
  • Minimizing pytorch usage outside of PytorchBackend and backends that require pytorch (solves CUDA_VISIBLE_DEVICES is not captured by torch #27)
  • DDP control arguments were moved to the pytorch backend for consistency.
  • As much as possible of the processing of hydra configs should happen in its __post_init__. Any other processing should not affect the types/values of the config

Some benefits of this refactoring:

  • All training benchmarks benefit from the warmup_steps through the measurement callback previously implemented for DDP.
  • solves CUDA_VISIBLE_DEVICES is not captured by torch #27.
  • Deprecation warnings for some arguments that will be removed soon in favor of more flexible arguments: generate_config instead of new_tokens, forward_config instead of num_images_per_prompt or any other argument that can be passed to diffusion pipelines.

Only breaking change is DDP.

@fxmarty fxmarty self-requested a review August 24, 2023 19:34
Copy link
Contributor

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I think it is indeed nicer to dissociate as much as possible the backends, and to use __post_init__ when possible. Still a bit worried for the possible change of defaults (like in transformers: huggingface/transformers#25237 (comment)), but I'm not sure if it is an issue or not.

I did not review everything but left a few comments!

Comment on lines 118 to 120
experiment = OmegaConf.to_container(
experiment, structured_config_mode=SCMode.INSTANTIATE
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve is False by default for to_container while it is True for to_object. Does this indeed resolves? Why and where?

I am not sure to remember why I put OmegaConf.create, why is it not necessary anymore?

Copy link
Member Author

@IlyasMoutawwakil IlyasMoutawwakil Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes true, to_object is safer and more strict, some things can go wrong unnoticed with to_container.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's no benefit to using to_create because with it we just go back to DictConfig objects.
not sure why we had it there, I thought it was part of the __post_init__ patch.

@IlyasMoutawwakil
Copy link
Member Author

IlyasMoutawwakil commented Aug 28, 2023

@fxmarty I added a tag before-refactoring that can be used in case this creates any issues.
I''l merge this once tests, pass.

@IlyasMoutawwakil IlyasMoutawwakil merged commit 82b0e53 into main Aug 28, 2023
IlyasMoutawwakil added a commit that referenced this pull request Aug 28, 2023
@IlyasMoutawwakil IlyasMoutawwakil deleted the refactoring branch April 30, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants