-
Notifications
You must be signed in to change notification settings - Fork 129
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
Port the compiler gym service backends to the new API #277
Port the compiler gym service backends to the new API #277
Conversation
fbd2af0
to
d337475
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I still think that not making the user know anything about what RPC mechanism you are using would be better, including allowing a simpler API for them.
} | ||
std::vector<ActionSpace> getActionSpaces() const final override { | ||
ActionSpace space; | ||
space.set_name("default"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shame these are protobufs, then they could just be params to the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that this class is never instantiated by the user. They pass it into CompilerGymService<T>
which handles lifetime management:
https://github.com/facebookresearch/CompilerGym/blob/development/compiler_gym/service/runtime/CompilerGymServiceImpl.h#L64
scope="module", | ||
params=[example.EXAMPLE_CC_SERVICE_BINARY, example.EXAMPLE_PY_SERVICE_BINARY], | ||
) | ||
def bin(request) -> Path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.fixture
decorator declares a fixture, which means that any test function can then take bin
as an argument and it will run and return the fixture value
Thanks for the review @hughleat! The CI is complaining of a problem that I've never seen before. An undefined reference to the destructor of
Will investigate and fix before merging. |
9fbb74e
to
f8a1085
Compare
I have been blocked on this issue for a couple of days now and cannot seem to figure it out. I have been unable to reproduce this locally on any build environment, including the docker environment used for building the linux wheels. The problem affects all targets that use the new C++ CompilerGymService implementation, and produces the same link-time error:
|
90e5b7a
to
abe23a4
Compare
Supporting the Fork() operator is optional for CompilationSessions, so provide a fallback implementation that creates a new environment and replays the action sequence by hand.
Encourage re-use of code within rules, and allow the test target to be overriden from the command line.
process.poll() may return 0.
Use SIGTERM rather than SIGKILL to terminate backend services, and then check that the process ended with returncode zero.
This adds a SIGTERM handler which enables the gRPC server to be shutdown gracefully. This enables an address sanitized build to run the end-of-process leak check.
30f373f
to
3cdb0e5
Compare
3cdb0e5
to
c2d511f
Compare
39916c6
to
4187b9d
Compare
Fixed the destructor linker error by rewriting the code using for loops rather than direct vector assignment. There's still a few bits to fix up in the CI configs then I'll merge. |
Codecov Report
@@ Coverage Diff @@
## development #277 +/- ##
===============================================
- Coverage 82.07% 81.84% -0.23%
===============================================
Files 87 87
Lines 4731 4765 +34
===============================================
+ Hits 3883 3900 +17
- Misses 848 865 +17
Continue to review full report at Codecov.
|
Try and keep all jobs in a workflow the same. E.g. don't bundle code coverage along with unit tests, etc.
4187b9d
to
f4fe600
Compare
All test signals green. Merging! |
Issue #254.