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

Added support for loading URLs in CompilerEnvStateReader.read_paths() #692

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

thecoblack
Copy link
Contributor

Fixes #148 Hi! This PR adds support for loading content from URLs. Regarding the tests, CompilerEnvStateReader.read_paths does not have any coverage yet, so I have added tests for other method features like input pipe or paths. LMK what you think :)!

@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 May 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 29, 2022

Codecov Report

Merging #692 (f105d45) into development (9ad5fbb) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development     #692      +/-   ##
===============================================
- Coverage        88.64%   88.61%   -0.03%     
===============================================
  Files              131      131              
  Lines             7926     7934       +8     
===============================================
+ Hits              7026     7031       +5     
- Misses             900      903       +3     
Impacted Files Coverage Δ
compiler_gym/compiler_env_state.py 100.00% <100.00%> (ø)
compiler_gym/envs/llvm/datasets/cbench.py 79.42% <0.00%> (-1.09%) ⬇️

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 9ad5fbb...f105d45. Read the comment docs.

@ChrisCummins
Copy link
Contributor

Hi @thecoblack, thanks for the patch! Your code looks clean and I appreciate the tests, thank you.

It looks like there is a missing module import. Couple of other minor suggestions left inline.

Cheers,
Chris

compiler_gym/compiler_env_state.py Show resolved Hide resolved
tests/compiler_env_state_test.py Outdated Show resolved Hide resolved
tests/compiler_env_state_test.py Outdated Show resolved Hide resolved
@ChrisCummins
Copy link
Contributor

LGTM, thanks @thecoblack!

Cheers,
Chris

@ChrisCummins ChrisCummins merged commit 13a0d34 into facebookresearch:development Jun 10, 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 loading URLs to CompilerEnvStateReader.read_paths()
4 participants