Skip to content

Warn if using offline data buffer without Agent wrapper #1771

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.

Sign up for GitHub

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

Open
QuantuMope wants to merge 3 commits into
base: pytorch
Choose a base branch
from
Open

Warn if using offline data buffer without Agent wrapper #1771

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74240c4
[Bug Fix] Enable hybrid SAC training
QuantuMope May 12, 2025
2c7e7cf
address comments
QuantuMope May 12, 2025
9db5652
address comments
QuantuMope May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions alf/algorithms/algorithm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1369,6 +1369,11 @@ def train_step_offline(self, inputs, state, rollout_info, pre_train=False):
"""
try:
if isinstance(rollout_info, BasicRolloutInfo):
logging.log_first_n(
logging.WARNING,
"Detected offline buffer training without Agent wrapper. "
"For best compatibility, it is advised to use the Agent wrapper.",
n=1)

@hnyu hnyu May 13, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This warning won't work. When using Agent, we still get rollout_info as BasicRolloutInfo.

@QuantuMope QuantuMope May 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It will. When using Agent, it properly feeds the nested BasicRLInfo to this function instead.

@QuantuMope QuantuMope May 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In other words, there was never a bug with hybrid RL training, just that it was never meant to be used without using Agent.

@hnyu hnyu May 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It will. When using Agent, it properly feeds the nested BasicRLInfo to this function instead.

you're right. I didn't know Agent overwrites this function itself.

rollout_info = rollout_info.rl
return self.train_step(inputs, state, rollout_info)
except:
Expand Down
10 changes: 4 additions & 6 deletions alf/algorithms/sac_algorithm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@
from alf.algorithms.config import TrainerConfig
from alf.algorithms.off_policy_algorithm import OffPolicyAlgorithm
from alf.algorithms.one_step_loss import OneStepTDLoss
from alf.data_structures import TimeStep, LossInfo, namedtuple, \
BasicRLInfo
from alf.data_structures import TimeStep, LossInfo, namedtuple
from alf.data_structures import AlgStep, StepType
from alf.nest import nest
import alf.nest.utils as nest_utils
Expand Down Expand Up @@ -845,9 +844,8 @@ def _select_q_value(self, action, q_values):
return q_values.gather(2, action).squeeze(2)

def _critic_train_step(self, observation, target_observation,
state: SacCriticState,
rollout_info: SacInfo | BasicRLInfo, action,
action_distribution):
state: SacCriticState, rollout_info: SacInfo,
action, action_distribution):

critics, critics_state = self._compute_critics(
self._critic_networks,
Expand Down Expand Up @@ -899,7 +897,7 @@ def _alpha_train_step(self, log_pi):
return sum(nest.flatten(alpha_loss))

def train_step(self, inputs: TimeStep, state: SacState,
rollout_info: SacInfo | BasicRLInfo):
rollout_info: SacInfo):
assert not self._is_eval
self._training_started = True
if self._target_repr_alg is not None:
Expand Down