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

Add support for granular timeouts #283

Closed
4 tasks
ChrisCummins opened this issue May 20, 2021 · 6 comments · Fixed by #716
Closed
4 tasks

Add support for granular timeouts #283

ChrisCummins opened this issue May 20, 2021 · 6 comments · Fixed by #716
Assignees
Labels
Enhancement New feature or request good first issue Good for newcomers

Comments

@ChrisCummins
Copy link
Contributor

ChrisCummins commented May 20, 2021

🚀 Feature

The compiler environment uses timeouts on all operations. These timeouts are controlled by a ConnectionOpts class, which is default-constructed by the constructor. This feature request proposes adding the ability for user code to override these timeouts on a per-method basis by passing in an timeout kwarg to operations that can timeout.

Motivation

Timeout specification is currently very coarse-grained. E.g. ConnectionOpts. rpc_call_max_seconds specifies the timeout of all RPC operations, regardless of what the operation is, the size of the benchmark that is being processed, etc. A timeout that would be suitable for a fast operation like ending a session would be totally unsuitable for a slow operation like applying a long sequence of expensive actions on a large benchmark. Currently the only way to tweak these timeouts to match the different calls is to manually change the env.service.opts fields in between method calls, e.g.:

>>> env = gym.make("llvm-v0", connection_settings=ConnectionOpts(rpc_call_max_seconds=10))
>>> env.reset()
>>> env.connection.opts.rpc_call_max_seconds = 60
>>> env.step(0)
>>> env.connection.opts.rpc_call_max_seconds = 10
>>> env.reset()

Pitch

Add an optional timeout kwarg so that timeouts can be overriden on a per-method basis:

>>> env = gym.make("llvm-v0")
>>> env.reset(timeout=10)
>>> env.step(0, timeout=60)
>>> env.reset(timeout=10)

The following methods would receive this new argument:

  • compiler_gym.envs.CompilerEnv.step
  • compiler_gym.envs.CompilerEnv.multistep
  • compiler_gym.envs.CompilerEnv.reset
  • compiler_gym.envs.CompilerEnv.__init__

All subclasses / wrappers would need to handle this too.

Alternatives

We could also consider ditching the ConnectionOpts class entirely and pushing all of the configurable options out to the individual methods, but I see there are two drawbacks to this approach:

  1. It makes it more difficult to support the use case where we don't need granular timeout control, e.g. where the usere would just like to specify a longer timeout that can be applied to all operations.
  2. Constructor timeouts would still need to saved so that they can used in fork() to propagate the same settings.
@ChrisCummins ChrisCummins added the Enhancement New feature or request label May 20, 2021
@ChrisCummins ChrisCummins added the good first issue Good for newcomers label Jun 4, 2021
@Hrittik20
Copy link

Hi @ChrisCummins, I would like to help fix this issue. Is this only applicable for "llvm_env_test.py" or other files as well?

@ChrisCummins
Copy link
Contributor Author

Hi @Hrittik20, great, thanks for your interest!

This task will require updating a number of methods. I've updated the "Pitch" section to list the main ones, which you will find in compiler_gym/envs/compiler_eny.py, but there are also subclasses and wrappers elsewhere that would need updating.

Cheers,
Chris

@ricardoprins
Copy link
Contributor

Hi @ChrisCummins, I'd like to work on this.

@ChrisCummins
Copy link
Contributor Author

Great, thanks @ricardoprins!

@ricardoprins
Copy link
Contributor

Hi,

In defining the kwarg will we want to preserve a timeout default of 300, as it is here?

class ConnectionOpts(BaseModel):
"""The options used to configure a connection to a service."""
rpc_call_max_seconds: float = 300

@ChrisCummins
Copy link
Contributor Author

Yep, that's right!

Cheers,
Chris

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants