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

Overhaul of the Python Agents Settings - Unified and transparent settings. #7209

Draft
wants to merge 32 commits into
base: dev
Choose a base branch
from

Conversation

Daraan
Copy link
Contributor

@Daraan Daraan commented Mar 5, 2024

Description

Currently the agents settings are mostly stored in arguments after initialization.
The user currently has to configure the agents in multiple ways: function parameters, a dictionary, a look-up class for the behavior agent.
After initialization some of the settings are permanent and cannot be changes anymore as no setter functions are implemented.

This updated addresses these points with two main goals:

  • Unification of the settings in one central data structure. Up to discussion a hierarchical one. I propose dataclasses.
  • Exposing (nearly) all the settings and allowing for dynamic changes of the settings

Additionally:

  • (nearly) all settings are annotated via docstrings, making them more clear for newcomers.
  • Building up from these changes de/serialization of an agent's settings is possible (e.g. to/from YAML).
  • Future work or up to users: The settings can be ported to a OmegaConf based configuration. Allowing to use its features like type-safety to detect invalid configurations early, or the usage of interpolations.

Where has this been tested?

  • Platform(s): Not relevant. Python code only
  • Carla version(s): 0.9.14, 0.9.15
  • Python version(s): 3.7, 3.10
  • Unreal Engine version(s): 4.26

Possible Drawbacks

Compatibility is not a given. With some setters and getters this can be overcome.
The agents_settings_backend.py is at some parts a bit involved to allow for a simple user interface. Namely the AgentConfig.update & __post_init__ to merge the overwrites dict, and the SimpleConfig that allows for flat and non-dataclass configs similar to the given BehaviorAgent behavior_types.

Todos & Discussions

  • Keep Python 2.7 compatibility? Which should be the minimum supported version.
  • Type Hints in signature need cleaning, depending on which version to support for correct Syntax.
  • Add an bridging-code to not break existing code. Add deprecation warning, or try to keep both interfaces?
  • Flat or hierarchical settings, as proposed?
  • Settings Backend
    • Functionality to port from/to OmegaConf and YAML
    • Discussion: More simple? What should be changed?
  • Fix & Complete documentation

This change is Reviewable

@glopezdiest
Copy link
Contributor

After a quick check, this looks like a promising PR. I'll take a deeper look as soon as possible, and report back

@glopezdiest
Copy link
Contributor

Hey, after a longer review here is my feedback.

First of all, the overall structure is a win for the agent parameters, as everything is localized onto one configuration file, which can be easily expanded over time.

However, the way the Config classes are created seems unnecessarily convoluted. The separation of the configuration in several data classes makes it very hard to keep track of the configuration format, and the amount of documentation and typing each parameter has also makes it confusing to follow.

Overall, the PythonAPI agents are meant to be a simple, easy to pick up module, and while the agent's are now simpler. the configuration is not.

@Daraan
Copy link
Contributor Author

Daraan commented May 14, 2024

Thank you for you feedback and I agree with you, I think some parts can be stripped and I have ideas how to simplify it. I'll look at it in the coming weeks again.

@Daraan Daraan force-pushed the overhaul-agent-parameters branch from a1ad11e to f9bc71d Compare June 21, 2024 14:10
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.

2 participants