Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@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

Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@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

Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@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

Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@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

Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@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

Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@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

Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@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

Lasertag self play - #26

Open
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play
Open

Lasertag self play#26
RPegoud wants to merge 57 commits into
RyanNavillus:mainfrom
RPegoud:lasertag_self_play

Conversation

@RPegoud

Copy link
Copy Markdown
Collaborator

New features:

  • Parallel PZ wrapper for Lasertag
  • Selfplay Curriculum
  • PPO training script using selfplay on lasertag
classSelfPlay(Curriculum):
def__init__(self, agent, device: str, store_agents_on_cpu: bool=False):
self.store_agents_on_cpu=store_agents_on_cpuself.storage_device="cpu"ifself.store_agents_on_cpuelsedeviceself.agent=deepcopy(agent).to(self.storage_device)
defupdate_agent(self, agent):
self.agent=deepcopy(agent).to(self.storage_device)
defget_opponent(self):
# Always return the most recent agentreturnself.agentdefsample(self, k=1):
return0
image

@RPegoud
RPegoud marked this pull request as draft March 28, 2024 14:03

@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.

Looks great! I left a couple of comments and suggestions, I'll message you on discord about the FSP/PFSP API

Comment threadlasertag/register.py
Comment threadlasertag_ppo.py Outdated
self.task = None
self.episode_return = 0
self.task_space = TaskSpace(spaces.MultiDiscrete(np.array([[2], [5]])))
self.possible_agents = np.arange(self.n_agents)

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.

Ideally this should be a list of agent names

Suggested change
self.possible_agents=np.arange(self.n_agents)
self.possible_agents=[f"agent_{i}"foriinrange(self.n_agents)]

Comment threadlasertag_ppo.py
out = {}
for idx, i in enumerate(array):
out[str(idx)] = i
return out

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.

Could these variable names be more descriptive? Also I assume str(idx) should instead be self.possible_agents[idx] if you change that to strings.

Comment threadlasertag_ppo.py Outdated
"""
Broadcasts the `done` and `trunc` flags to dictionaries keyed by agent id.
"""
return {str(idx): value for idx in range(self.n_agents)}

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.

Same thing here, maybe just calling idx agent_idx is enough

Comment threadlasertag_ppo.py Outdated
action = batchify(action, device)
obs, rew, done, info = self.env.step(action)
obs = obs["image"]
trunc = 0 # there is no `truncated` flag in this environment

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
trunc=0# there is no `truncated` flag in this environment
trunc=False# there is no `truncated` flag in this environment

Comment threadlasertag_ppo.py
probs = Categorical(logits=logits)
if action is None:
action = probs.sample()
return action, probs.log_prob(action), probs.entropy(), self.critic(hidden)

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.

Didn't review the agent code super closely but it looks good

Comment threadlasertag_ppo.py Outdated
# convert to torch
obs = torch.tensor(obs).to(device)

return obs

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.

Is there any reason not to just use batchify? They do the exact same thing right?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

That's right, the original PettingZoo code had an added obs = obs.transpose(0, -1, 1, 2) in batchify_obs which I removed. So there's no need for this function anymore

Comment threadlasertag_ppo.py Outdated
x = x.cpu().numpy()
x = {a: x[i] for i, a in enumerate(env.possible_agents)}

return x

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 assume this duplicated code isn't intentional

Comment threadlasertag_ppo.py Outdated
print(f"Approx KL: {approx_kl.item()}")
print(f"Clip Fraction: {np.mean(clip_fracs)}")
print(f"Explained Variance: {explained_var.item()}")
print("\n-------------------------------------------\n")

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 could consider adding in weights and biases integration (it's only like 20 lines of code, check out cleanrl_procgen_plr.py). It's a good tool and generates plots for you automatically, and most RL tools integrate with it. You'll need to learn it eventually for this project, but if you're happy with how you're doing things you can try wandb out later.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds like a good improvement, I just used whatever logging was used in the cleanRL script but wandb should provide a clearer overview of training progress

Comment threadsyllabus/task_space/task_space.py

@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.

Could you put the agent curricula into a file syllabus/curricula/selfplay.py? Also please move the DualCurriculumWrapper to syllabus/core/dual_curriculum_wrapper.py.

@RPegoud
RPegoud marked this pull request as ready for review April 18, 2024 15:14

@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.

Thanks for moving everything into Syllabus, looks great!. Could you also add some test cases to the multiagent smoke tests (maybe using pettingzoo's chess environment) to test the self play algorithms. We just want something quick that runs through their code to make sure there are no basic runtime errors

self.agent_mp_curriculum, self.agent_task_queue, self.agent_update_queue = (
make_multiprocessing_curriculum(agent_curriculum)
)
self.sample() # initializes env_task and agent_task

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 initializer seems hacky. I don't think we should be internally making multiprocessing curricula. Instead let the user create 2 curricula, pass them to this, then in their code, wrap the dual curriculum in a multiprocessing wrapper. This wrapper should implement update functions to pass updates to both the agent and environment curriculum. For example the update_on_step function should call self.env_curriculum.update_on_step and self.agent_curriculum.update_on_step.

I also don't see why we need to call sample here?

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.

The initializer might also need to update the task space. So the task space of this curriculum should be Tuple(env_curriculum.task_space, agent_curriculum.task_space)

I'll try to merge Nistha's task space updates in today so that you can pull them in here

Comment threadsyllabus/curricula/selfplay.py Outdated
Comment threadsyllabus/curricula/selfplay.py Outdated
its priority.
"""
if self.n_stored_agents < self.max_agents:
# TODO: define the expected behaviour when the limit is exceeded

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.

We probably should delete or overwrite agents when this happens.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Then I suggest this:

joblib.dump(
agent,
filename=f"{self.storage_path}/{self.name}_agent_checkpoint_{self.current_agent_index%self.max_agents}.pkl",
)
self.current_agent_index+=1

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.

If we go this route, you should save the most recent agent to a file some.where. Since it won't be obvious from the filenames. It might be better to just delete a file and write a new one, but this is fine for now.

@RyanNavillus
RyanNavillus changed the base branch from nmmo to lasertagMay 21, 2024 21:27
@RyanNavillus
RyanNavillus changed the base branch from lasertag to mainNovember 24, 2024 08:10
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

@RPegoud@RyanNavillus