Skip to content
This repository was archived by the owner on May 27, 2026. It is now read-only.

Add support for granular timeouts#716

Merged
ChrisCummins merged 7 commits into
facebookresearch:developmentfrom
mar-galho:development
Jun 24, 2022
Merged

Add support for granular timeouts#716
ChrisCummins merged 7 commits into
facebookresearch:developmentfrom
mar-galho:development

Conversation

@mar-galho

Copy link
Copy Markdown
Contributor

Fixes #283.

  • Added timeout kwarg to CompilerEnv and related classes.
  • Added deprecation note to rpc_call_max_seconds in ConnectionOpts.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 16, 2022

@ChrisCummins ChrisCummins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey Ricardo,

Thanks for working on this! I left a couple of comments inline. Also, it would be great if you add tests to cover this new param, perhaps by mocking the underling call to env.service to check that the parameter is forwarded correctly.

Please ignore the failing CI/www jobs, it is a known issue and can be ignore #709.

Cheers,
Chris

Comment thread compiler_gym/envs/compiler_env.py Outdated
Comment thread compiler_gym/service/connection.py Outdated
@codecov-commenter

codecov-commenter commented Jun 20, 2022

Copy link
Copy Markdown

Codecov Report

Merging #716 (b533824) into development (444b82f) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development     #716      +/-   ##
===============================================
- Coverage        88.72%   88.63%   -0.09%     
===============================================
  Files              131      131              
  Lines             7935     7936       +1     
===============================================
- Hits              7040     7034       -6     
- Misses             895      902       +7     
Impacted Files Coverage Δ
compiler_gym/service/connection.py 77.59% <ø> (-1.08%) ⬇️
compiler_gym/wrappers/core.py 81.92% <ø> (ø)
compiler_gym/envs/compiler_env.py 75.44% <100.00%> (ø)
...ompiler_gym/service/client_service_compiler_env.py 90.47% <100.00%> (-0.38%) ⬇️
compiler_gym/views/observation_space_spec.py 82.85% <0.00%> (-2.86%) ⬇️
compiler_gym/views/observation.py 97.29% <0.00%> (-2.71%) ⬇️
...loop_tool/service/loop_tool_compilation_session.py 87.83% <0.00%> (-2.03%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 444b82f...b533824. Read the comment docs.

@mar-galho

Copy link
Copy Markdown
Contributor Author

I'll work on the tests.

Should I add them to the compiler_env_test.py file only? It seems that the connection_test.py file is already covering the timeout kwarg, so I am assuming that, like you said, we should only check whether the kwarg is being passed through to the CompilerGymServiceConnection call...am I right?

@ChrisCummins

Copy link
Copy Markdown
Contributor

Thanks @ricardoprins!

Should I add them to the compiler_env_test.py file only? It seems that the connection_test.py file is already covering the timeout kwarg, so I am assuming that, like you said, we should only check whether the kwarg is being passed through to the CompilerGymServiceConnection call...am I right?

Yep, that sounds like a good idea.

Cheers,
Chris

Comment thread compiler_gym/service/client_service_compiler_env.py Outdated
Comment thread compiler_gym/wrappers/core.py Outdated
@ChrisCummins

Copy link
Copy Markdown
Contributor

LGTM, thanks @ricardoprins!

Cheers,
Chris

@ChrisCummins ChrisCummins merged commit 51d903e into facebookresearch:development Jun 24, 2022
This was referenced Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for granular timeouts

4 participants