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 #716

Merged
merged 7 commits into from Jun 24, 2022

Conversation

ricardoprins
Copy link
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
Copy link
Contributor

@ChrisCummins ChrisCummins left a comment

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

compiler_gym/envs/compiler_env.py Outdated Show resolved Hide resolved
compiler_gym/service/connection.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2022

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.

@ricardoprins
Copy link
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
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

@ChrisCummins
Copy link
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 join this conversation on GitHub. Already have an account? Sign in to comment
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