Refactor of Curriculum and parameter sampling

[vincentpierre]

Proposed change(s)

So far:

• parameter_randomization and curriculum will both be under the same config category "environment_parameters"
• Simplified the logic in trainer_controller
• Removed Curriculum and MetaCurriculum
• Upgraded the existing config files
• Upgraded the upgrade script

TODO :

• Documentation / Migrating / Changelog
• Code simplifications

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

Refactor of Curriculum and parameter sampling as described in this document

Types of change(s)

• Bug fix
• New feature
• Code refactor
• Breaking change
• Documentation update
• Other (please describe)

Checklist

• Added tests that prove my fix is effective or that my feature works
• Updated the changelog (if applicable)
• Updated the documentation (if applicable)
• Updated the migration guide (if applicable)

Other comments

[ervteng]

I think this generally should match the YAML structure/keys, otherwise it won't be able to be dumped. So in this case this class should contain curriculum: List[Lesson].

[vincentpierre]

I create these using the structure method, not the default converter. This code works as intended.
I did not name it curriculum because it can sometimes contain a single lesson (not a curriculum wen there is a single value).

[ervteng]

IMO it's kind of defeating the purpose of the attrs classes if they don't match the YAML. In the other classes doing an unstructure of the entire RunOptions class structure creates a usable YAML file.

Hence I think the environment_parameters dict should contain as the entries either this object (which contains curriculum) or a Lesson object if it only has a single lesson.

[vincentpierre]

I think I see what you are saying. And I think it is related to your other comment. I am trying to make the EnvironmentParameterSetting agnostic of how the parameters where written in the yaml. This allows me to have much simpler logic in the update_lesson method since I know I always have a list of lessons, always with a completion criteria and a sampler. But in the yaml, there are multiple ways to specify a sampler :

small_wall_height:
    curriculum:
    - Lesson0:
        value: 1.5
small_wall_height: 1.5
other_wall_height:
  sampler_type: uniform
  sampler_parameters:
    min_value: 1.50
    max_value: 1.51

But all must result in a list of lessons. I could have the settings mirror the yaml, but then I would need to write a converter from the settings to the list of Lessons.
Do you think there is a way to both mirror the yaml and convert it to a list of lessons ?
Or do you think I should not have a list of lessons to begin with and have my logic be case by case (curriculum / sampler / constant) ?

[vincentpierre]

On WallJump, the sampling now happens using the sampler feature

[vincentpierre]

This was an attempt at organizing the sections of this file better. I can remove if it does not deliver.

[andrewcoh]

So, everything under environment_parameters gets converted into a Lesson. My concern with this is that this sets the precedent that every new env parameter controller needs to go through the control for curriculum. It's not obvious how this might extend to things like an adaptive sampling scheme that don't have predefined completion criterion or something like Ervin and Scott's task parameterizations.

[ervteng]

I actually think this is OK. A new sampler type (like adaptive, or active learning, etc.) would be set as a Value in the 1st lesson. Running it through curriculum would allow us, for instance, to do multiple steps (e.g. Lesson 0 is a constant value, Lesson 1 is a regular Sampler, Lesson 2 is an adaptive sampler).

[andrewcoh]

So we are basically saying that all future environment parameter controllers will be controlled as curriculums?

[vincentpierre]

We can always modify environment parameter as needed but sampling was easy to fold as curriculum. I think for environment parameters, whatever method we find for generating the parameters, it should be possible to fold it into a curriculum, even if it has only one lesson.

[andrewcoh]

My feeling is (1) this unnecessarily incurs the cost of being a curriculum (i.e. the extra function calls in the EnvParMan) when it doesn't need to be and (2) environment parameter controllers may use update rules that are not as rigid as curriculum update thresholds or depend on something other metric than progress/reward.

[vincentpierre]

(1) The cost of being a curriculum allows us to have a much simpler interface of the EnvironmentParameterManager. I agree this is a (very small) added cost of computation but reusing code is important IMO.
(2) Nothing prevents us to make the completion criteria evolve in the future to fit a future use case or create adaptive samplers. We can also add new fields and methods to the EnvironmentParameterManager later if we need, but for this PR, formulating the samplers as one lesson curriculum seemed like an opportunity for code simplification (since curriculum now support sampling).
We can try to edit the design doc with an alternative to this implementation?
Would the EnvironmentParameterSetting have an optional list of lessons and an optional sampler instead of a list of lessons ?

[chriselion]

Can we return an empty EnvironmentParameterManager in this case? I think it would simplify some of the None checks in TrainerController.

[chriselion]

For example, this code could get simpler if we knew param_manager was non-None

[chriselion]

Why make this static? lesson.completion_criteria.need_increment(...) feels more natural.

[chriselion]

Small optional feedback, looks good otherwise.

[andrewcoh]

We discussed some future ideas for this feature in an offline slack thread but nothing that should block this.