[RLlib; Offline RL] Implement CQL algorithm logic in new API stack.

[simonsays1980]

Why are these changes needed?

This PR proposes a training logic for CQL in our new API stack using RLModules, Learner API, Offline RL API, and EnvRunner API. More specifically CQL on the new stack uses OfflineData to read and batch training data and RLModules and Learner API to define and train a policy. More specifically, it inherits most of its model logic from SAC as it implements the entropy version of CQL. To add NEXT_OBS to the batch it overrides the AlgorithmConfig's build_learner_connector and proposes therewith a new form how the learner connector should be modified (in contrast to adding more connectors to the learner's pipeline in the learner's build method.

Furthermore, this PR adds a "tuned example" using Pendulum-v1 that shows how CQL can be used on the new API stack.

This PR is a part of a sequence of PRs porposed and coming.

Related issue number

Relates to #46969 and closes #37779

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[sven1977]

size="large" is better, no?

[simonsays1980]

Yeah, maybe. It takes around 50 iterations.

[simonsays1980]

Will tune this in another PR.

[sven1977]

Suggested change

	# Add a metric to keep track of training iterations to
	# determine when switching the actor loss from behavior
	# cloning to SAC.
	self.metrics.log_value(
	(ALL_MODULES, TRAINING_ITERATION), float("nan"), window=1
	)
	# Add a metric to keep track of training iterations to
	# determine when switching the actor loss from behavior
	# cloning to SAC.
	self.metrics.log_value(
	(ALL_MODULES, TRAINING_ITERATION), float("nan"), window=1
	)

[sven1977]

This can all go, b/c we now use the default=0 arg.

[sven1977]

I feel like it's better to add the NEXT_OBS LearnerConnector piece here, what do you think?

The CQLLearner is the component that has this (hard) requirement (meanin it won't work w/o this connector piece), so it should take care of adding it here.
Similar to how it would check, whether the used RLModule fits a certain API, if required.

[simonsays1980]

I agree that the Learner is the component that needs the connectors. On the other side it uses the algorithm.build_learner_connector to build its connector pipeline. So, I am a bit unsure, where we should generally pack it.

In this specific case I needed to add it in the build_learner_connector b/c the class inheritance was somehow avoiding it otherwise (i.e. the CQLLearner had it, but the TorchCQLLearner did not.

[sven1977]

Thanks for clarifying this! Always better to be expressive :)

[sven1977]

👍

[sven1977]

👍

[sven1977]

If we change the test to "size=large", would the return get a bit higher?

[simonsays1980]

I haven't tried this. Will do.

[sven1977]

see my comment in CQLLearner. I think we should move this there. Same as for DQN. Or if there are good arguments to leave these in the Config classes, we should also change it in DQN/SAC.

[simonsays1980]

As mentioned above. In rare cases the class inheritance avoids it being changed there and changing it in build_learner_connector would then still work.

You are right that we need to unify the way of adding them in all algorithms (also MARWIL)

[sven1977]

It just doesn't feel clean, doing it here. The config classes should contain as little (actually implemented) algo logic as possible and just store settings.

I know I sound like a broken record, but "separation of concerns" principle :):

• Whatever an algo-specific Learner must-have, it should create this in its build() method, for example a lr-schedule, or a kl-coeff-schedule, etc..
• Whatever an algo-specific Learner must-have, it should check that it has this in its build() method, e.g. "does my RLModule implement API xyz?"
• Algo configs should NOT implement any algo logic. If they still do somewhere for some algos, we should add a TODO to move the logic into a more appropriate location.

Suggestion:
Can we try creating a CQLLearner class that inherits from SACLearner and overrides the build() method just to insert that connector piece? Then do class CQLTorchLearner(SACTorchLearner, CQLLearner):. That should work, no?