sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

sb3 - #38

Open
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr
Open

sb3#38
AdrianHuang2002 wants to merge 32 commits into
RyanNavillus:mainfrom
AdrianHuang2002:sb3-progen-plr

Conversation

@AdrianHuang2002

Copy link
Copy Markdown

No description provided.

@AdrianHuang2002

Copy link
Copy Markdown
Author

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left some comments. Please make the requested changes to simplify the PR a bit, and let me know if you have any questions about the callbacks.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove any changes to this file

)
env = openai_gym.make(f"procgen-{env_id}-v0", distribution_mode="easy", start_level=start_level, num_levels=num_levels)
env = GymV21CompatibilityV0(env=env)
components = MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
components=MultiProcessingComponents(task_queue=task_queue, update_queue=update_queue)
components=curriculum.get_components()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached syllabus/examples/training_scripts/wandb/run-20240423_020001-cymykoqj/files/events.out.tfevents.1713852002.WenranLaoGong.157219.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this file git rm --cached profiling_results.prof

return mean_returns, stddev_returns, normalized_mean_returns


class CustomCallback(BaseCallback):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Look at the documentation here to change the behavior of the callback https://stable-baselines3.readthedocs.io/en/v1.0/guide/callbacks.html

Comment on lines +238 to +239
mean_eval_returns, _, _ = level_replay_evaluate_sb3(args.env_id, model, args.num_eval_episodes, num_levels=0)
writer.add_scalar("test_eval/mean_episode_return", mean_eval_returns, self.global_step)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code should only be run once every update. There is a different callback method _on_training_end that you should probably use.

If you need access to any data from training, try printing out self.locals or self.globals from within the callback method to see what is available

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked the hyperparameters you included, but I didn't review any that you excluded. I'll revisit that later

"""
return True

def _on_rollout_end(self) -> None:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think you can put this function in the CustomCallback rather than creating 2 separate ones

return True

def _on_rollout_end(self) -> None:
if self.num_timesteps % self.eval_freq == 0:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't necessary, we should just evaluate every time this function is called. It should happen every 16,000 steps, but try running it and make sure that's what happens

Comment on lines 199 to 202
def wrap_vecenv(vecenv):
vecenv.is_vector_env = True
vecenv = VecMonitor(venv=vecenv, filename=None)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True)
vecenv = VecNormalize(venv=vecenv, norm_obs=False, norm_reward=True, training=False)
return vecenv

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is used for both eval and training, we should probably pass training as an argument to wrap_vecenv, so that training=True for the training envs right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove these changes

n_epochs=3,
clip_range_vf=0.2,
ent_coef=0.01,
batch_size=256 * 64,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
batch_size=256*64,
batch_size=2048

batch_size is actually the minibatch size. We want 8 batches for 25*64 steps, so 2048 steps per minibatch


print("Creating model")
model = PPO(
"CnnPolicy",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You're going to need to find a way to replace this with the ProcgenAgent model. https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html

Take a look at the advanced example https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

I think if you replace the CustomNetwork with our Policy (the parent class of ProcgenAgent) then the code they have here might just work out of the box.

@RyanNavillusRyanNavillus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if this works, but it you should try this. It would be a lot simpler this way

fromsyllabus.examples.models.procgen_modelimportPolicyclassCustomActorCriticPolicy(ActorCriticPolicy):
def__init__(
self,
observation_space: gym.spaces.Space,
action_space: gym.spaces.Space,
lr_schedule: Callable[[float], float],
net_arch: Optional[List[Union[int, Dict[str, List[int]]]]] =None,
activation_fn: Type[nn.Module] =nn.Tanh,
*args,
**kwargs,
):
super(CustomActorCriticPolicy, self).__init__(
observation_space,
action_space,
lr_schedule,
net_arch,
activation_fn,
# Pass remaining arguments to base class*args,
**kwargs,
)
# Disable orthogonal initializationself.ortho_init=Falsedef_build_mlp_extractor(self) ->None:
self.mlp_extractor=Policy(...)

This is a small change to the documentation here https://stable-baselines3.readthedocs.io/en/v1.0/guide/custom_policy.html#advanced-example

return value, action_log_probs, dist_entropy


class Sb3ProcgenAgent(CustomPolicy):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think SB3's model will exclusively call forward, so this class isn't necessary

Comment on lines +49 to +59
def get_value(self, input):
value, _, _ = self.network(input)
return value

def evaluate_actions(self, input, rnn_hxs, masks, action):
value, actor_features = self.network(input, rnn_hxs, masks)
dist = self.dist(actor_features)

action_log_probs = dist.log_prob(action)
dist_entropy = dist.entropy().mean()
return value, action_log_probs, dist_entropy

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

See comment below, I'm not sure you need to add these methods

@AdrianHuang2002

Copy link
Copy Markdown
Author

Sign up for freeto 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

@AdrianHuang2002@RyanNavillus