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

[service] Add a CompilationSession abstract base class. #261

Merged

Conversation

ChrisCummins
Copy link
Contributor

The CompilationSession class encapsulates an incremental compilation
session.

This is the first in a series of patches that separates the RPC server
from the compilation session.

Issue #254.

@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 11, 2021
@ChrisCummins ChrisCummins force-pushed the compilation-session branch from f6b63e1 to b54a667 Compare May 11, 2021 19:25
@ChrisCummins ChrisCummins requested review from hughleat and bwasti May 11, 2021 19:26
@codecov-commenter
Copy link

Codecov Report

Merging #261 (f6b63e1) into development (5d20b14) will decrease coverage by 0.20%.
The diff coverage is n/a.

❗ Current head f6b63e1 differs from pull request most recent head b54a667. Consider uploading reports for the commit b54a667 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development     #261      +/-   ##
===============================================
- Coverage        83.33%   83.12%   -0.21%     
===============================================
  Files               76       76              
  Lines             4338     4338              
===============================================
- Hits              3615     3606       -9     
- Misses             723      732       +9     
Impacted Files Coverage Δ
compiler_gym/envs/compiler_env.py 86.33% <ø> (-0.59%) ⬇️
compiler_gym/service/proto/__init__.py 100.00% <ø> (ø)
compiler_gym/service/connection.py 74.16% <0.00%> (-2.59%) ⬇️

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 77d28bc...b54a667. Read the comment docs.

@ChrisCummins ChrisCummins force-pushed the compilation-session branch from b54a667 to efa8dc0 Compare May 11, 2021 23:42
@ChrisCummins
Copy link
Contributor Author

Marking this ready for review but no rush to merge it while we're still discussing the design in #254.

@ChrisCummins ChrisCummins force-pushed the compilation-session branch from efa8dc0 to 3ac01aa Compare May 12, 2021 12:39
The CompilationSession class encapsulates an incremental compilation
session.

This is the first in a series of patches that separates the RPC server
from the compilation session.

Issue facebookresearch#254.
@ChrisCummins ChrisCummins force-pushed the compilation-session branch from 3ac01aa to 9126512 Compare May 12, 2021 14:53
Copy link
Contributor

@hughleat hughleat left a comment

Choose a reason for hiding this comment

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

If the protobuf stuff is okay to keep then this LGTM :-)


def __init__(
self, working_dir: Path, action_space: ActionSpace, benchmark: Benchmark
):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: why doesn't this return a status like the c++ code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use exceptions in C++ (FWIW I don't have particularly strong opinions on the matter), the CompilationSession constructor in C++ is a safe default, and any of the three methods that can fail (init, compute feature, run action) return Status.

For Python, exceptions are fine and all of the runtime logic that interacts with CompilationSession will catch exceptions and set the relevant RPC error code

@ChrisCummins ChrisCummins merged commit 0aa1051 into facebookresearch:development May 13, 2021
@ChrisCummins ChrisCummins deleted the compilation-session branch May 13, 2021 15:04
This was referenced Jun 3, 2021
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.

4 participants