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

Run benchmark directly using agent locator #1846

Merged
merged 13 commits into from
Feb 9, 2023
Merged

Conversation

Adaickalavan
Copy link
Member

Runs the benchmark directly using an agent locator string, instead of going through a config file such as SMARTS/baselines/driving_smarts/v0/agent_config.yaml.

Addresses #1838 (comment) .

@Adaickalavan Adaickalavan requested a review from Gamenot February 9, 2023 06:45
Comment on lines +32 to +39
A list of SMARTS zoo agents which are compatible with this benchmark is
provided here. A compatible zoo agent can be run as follows.

.. code-block:: bash

$ cd <path>/SMARTS
$ scl zoo install <path/to/agent policy>
$ scl benchmark run driving_smarts==0.0 <agent_locator> --auto_install
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be a reminder of two things: we should set up a utility to determine the general compatibility of agents.

This is relevant to #381, #1602, and #1603 but we seem to be missing a specific issue.

And to add the validation test pre-benchmark to exclude invalid agents from use in the benchmark.

This is relevant to #1732

@@ -68,22 +68,20 @@ def run(

\b
<benchmark_id> is formatted like BENCHMARK_NAME==BENCHMARK_VERSION.
<agent_config> is the path to an agent configuration file.
<agent_locator> is the locator string for the registered agent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are using agent_locator now I think we should use smarts.core.utils.class_factory.is_valid_locator to test if the locator is correctly formatted.

We can use smarts.core.utils.class_factory.NAME_CONSTRAINT_REGEX.pattern.

something like:

from smarts.core.utils.class_factory import NAME_CONSTRAINT_REGEX

...
if not NAME_CONSTRAINT_REGEX.search(agent_locator):
    raise ValueError(
        f"Invalid locator={agent_locator}. Syntax must match regular expression: "
        "{NAME_CONSTRAINT_REGEX.pattern}"
    )

Copy link
Member Author

Choose a reason for hiding this comment

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

We have _raise_on_invalid_locator(locator) function guarding the ClassRegister.make(). So perhaps we don't have to check locator validity in other parts of the code?

def make(self, locator, **kwargs):
"""Calls the factory with `locator` name key supplying the keyword arguments as argument
overrides.
"""
factory = self.find_factory(locator)

def find_factory(self, locator):
"""Locates a factory given a locator."""
self._raise_on_invalid_locator(locator)

Copy link
Collaborator

@Gamenot Gamenot Feb 9, 2023

Choose a reason for hiding this comment

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

Fair enough, I was mentioning this since it is best practice to exit as early as possible. The factory side check waits all the way to the point where we call registry.make. That is after installing dependencies, reading configuration, and uncovering the environment.

The check is probably better to go through the registry interface instead similar to how gym works with the locator:

https://github.com/Farama-Foundation/Gymnasium/blob/76559ed553cffc3e5082227b0a8acea461273e84/gymnasium/envs/registration.py#L115-L136

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just thought we could hack it using NAME_CONSTRAINT_REGEX but that is perhaps inappropriate to use the regex object directly in case of changing the underlying implementation.

@Adaickalavan Adaickalavan merged commit ea28c20 into master Feb 9, 2023
@Adaickalavan Adaickalavan deleted the rm-baselines branch February 9, 2023 17:11
@Gamenot Gamenot mentioned this pull request Feb 9, 2023
3 tasks
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.

2 participants