Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Competition agent integration with zoo #1724

Closed
wants to merge 19 commits into from

Conversation

AisenGinn
Copy link
Contributor

@AisenGinn AisenGinn commented Nov 24, 2022

competition_agent integration with zoo.

run:scl run --envision competition_agent_demo.py scenarios/sumo/loop to see competition integration with loop scenarios. To see other scenarios, replace scenarios/sumo/loop with other scenarios path.

@Adaickalavan
Copy link
Member

  • Why is this pull request being merged into comp-1 branch?
  • We may grandfather comp-1 branch and add refactored competition code into develop branch such that it can run any external users' agents in general.

@AisenGinn
Copy link
Contributor Author

AisenGinn commented Dec 2, 2022

  • Why is this pull request being merged into comp-1 branch?
  • We may grandfather comp-1 branch and add refactored competition code into develop branch such that it can run any external users' agents in general.

Currently only comp-1 branch has the competition folder and the require policies/requirements. My work is based on that and merging directly into develop may cause some issues I think. Maybe we should try to merge comp-1 into develop first?

@Gamenot
Copy link
Collaborator

Gamenot commented Dec 2, 2022

  • We may grandfather comp-1 branch and add refactored competition code into develop branch such that it can run any external users' agents in general.

Currently only comp-1 branch has the competition folder and the require policies/requirements. My work is based on that and merging directly into develop may cause some issues I think. Maybe we should try to merge comp-1 into develop first?

It is possible that comp-1 does not get merged in. This agent, however, lets us run competition agents directly as were created in the competition so that we will not have to modify them.

@AisenGinn AisenGinn requested review from saulfield and removed request for Gamenot and qianyi-sun December 6, 2022 16:04
register(
"competition_agent-v0",
entry_point=competition_entry,
policy_path=os.path.join(root_path, "competition/track1/submission"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the default but we will want to remove this default before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we will want to either do it by referencing the winning agents like "competition_aid_v0", "competition_vcr_v0", and "competition_tju-fanta_v0". or by removing a default to force policy_path to be specified when initializing an agent.

Comment on lines 212 to 213
while sub_env_path in sys.path:
sys.path.remove(sub_env_path) # prevent duplicate path remain in sys.path
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this end up in the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sub_env_path is appended when the dependency is installed.

Comment on lines 209 to 210
while policy_path in sys.path:
sys.path.remove(policy_path) # prevent duplicate path remain in sys.path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it necessary to remove duplicates like this?

Copy link
Contributor Author

@AisenGinn AisenGinn Dec 8, 2022

Choose a reason for hiding this comment

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

There are some users that insert the submission path inside the policy.py they wrote, some do not. If they insert there will be two submission paths inside sys.path (which is a list that allow duplication). This is to remove all possible policy paths inside sys.path.

Copy link
Collaborator

@Gamenot Gamenot Dec 8, 2022

Choose a reason for hiding this comment

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

OK, that makes more sense please add a code comment about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Comment on lines 28 to 40
while self._sub_env_path in sys.path:
sys.path.remove(self._sub_env_path)
while self._policy_dir in sys.path:
sys.path.remove(self._policy_dir)
for key, module in list(sys.modules.items()):
if "__file__" in dir(module):
module_path = module.__file__
if module_path and (
self._policy_dir in module_path or self._sub_env_path in module_path
):
sys.modules.pop(key)
if remove_all_env:
shutil.rmtree(self._comp_env_path, ignore_errors=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This cleanup seems to be related to the requirements for all agents of this type rather than the agent instance-specific resources. Maybe this can be registered as an atexit callback in the entrypoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sound reasonable, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After trying to turn it into a callback function, I suggest to keep this as before. Removing environment using callback function become slower and the program may crash frequently.

@AisenGinn AisenGinn requested review from Gamenot and removed request for saulfield December 12, 2022 17:45
@Gamenot
Copy link
Collaborator

Gamenot commented Feb 14, 2023

This has been resolved with #1860, #1858, #1838

@Gamenot Gamenot closed this Feb 14, 2023
Sign up for free to 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.

4 participants