Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: update configs folder #1101

Merged
merged 9 commits into from
Oct 10, 2024
Merged

refactor: update configs folder #1101

merged 9 commits into from
Oct 10, 2024

Conversation

WiemKhlifi
Copy link
Contributor

What?

Updating the configs folder.

How?

  • Storing all defaults in one folder for Anakin systems.
  • Removing all system_name_logger and use one base_logger.
  • Update the config paths in systems & test files.

test/integration_test.py Outdated Show resolved Hide resolved
@WiemKhlifi WiemKhlifi self-assigned this Oct 9, 2024
@WiemKhlifi WiemKhlifi linked an issue Oct 9, 2024 that may be closed by this pull request
2 tasks
Copy link
Contributor

@sash-a sash-a left a comment

Choose a reason for hiding this comment

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

Thanks Wiem!

Couple small things:

  1. Just put the default configs in configs/default (no anakin) anakin/sebulba will share the same configs except for arch to start. If we find that params differ a lot then we can change it later. Because of this I think you can leave the integration tests as is
  2. I think remove the default_... part of the config file name because they're now in the default folder.

@WiemKhlifi
Copy link
Contributor Author

WiemKhlifi commented Oct 10, 2024

Thank you Sasha for these suggestions! Without the default_ is definitely cleaner I made all the necessary changes please check them 🙏

Copy link
Contributor

@OmaymaMahjoub OmaymaMahjoub left a comment

Choose a reason for hiding this comment

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

Thanks @WiemKhlifi for the changes, I added minor request 🙏

OmaymaMahjoub
OmaymaMahjoub previously approved these changes Oct 10, 2024
Copy link
Contributor

@OmaymaMahjoub OmaymaMahjoub left a comment

Choose a reason for hiding this comment

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

LGTM! 🔥

sash-a
sash-a previously approved these changes Oct 10, 2024
Copy link
Contributor

@sash-a sash-a left a comment

Choose a reason for hiding this comment

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

Thanks Wiem! Looks great 🏅

Copy link
Contributor

@OmaymaMahjoub OmaymaMahjoub left a comment

Choose a reason for hiding this comment

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

Thanks @WiemKhlifi 🙏

@sash-a sash-a merged commit 2a1d2d8 into develop Oct 10, 2024
4 checks passed
@sash-a sash-a deleted the feat/config_updates branch October 10, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Reduce config files
3 participants