gRPC refactoring of actions and observations #531
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
Looking good @sogartar! I'll keep the design discussion on this thread, so here's just my comments on the code. A few random thoughts:
-
Everything should live in one namespace. IMO
ActionSpaceis easier to read thanspaces.Action, and removes the risk of aliasing headaches when you import multiple headers in C++ / packages in Python. -
It took me a while to grok your replacements for the vector types (e.g.
DoubleList). I think your new proposal supports both fixed size boxes (Space.double_box) and variable length arrays (Space.double_sequence), which is nice, but it isn't super clear to me. Could you add some more comments? Is there a use case for variable-length arrays? -
I'm not sure about the file splits. It looks to me like
basic.protoandevent.protocould be merged into a singletensor.proto, andspaces.protocontains range types which are not spaces.
Cheers,
Chris
|
@ChrisCummins, thanks for the quick reply. I merged all proto files into a single one as was before.
I added a comment about that. |
sogartar
force-pushed
the
grpc-refactoring
branch
from
f076d3e
to
820bf31
Compare
Codecov Report
@@ Coverage Diff @@
## development #531 +/- ##
===============================================
+ Coverage 87.49% 87.77% +0.28%
===============================================
Files 113 114 +1
Lines 6411 6601 +190
===============================================
+ Hits 5609 5794 +185
- Misses 802 807 +5
Continue to review full report at Codecov.
|
|
@ChrisCummins, this PR is ready for review. Tests, benchmarks and examples should pass. Unresolved issues:
The dispatching based on |
|
Hi @sogartar, great work as always, thank you! It's interesting to see that the IIUC, you're using // These wrapper messages enable the `<Type>Range` ranges to
// have optional upper and lower bounds.
message OptionalBoolean {
bool value = 1;
}
message OptionalDouble {
double value = 1;
}
// ... Int64, FloatYou can then use e.g. I will try and work through the rest of the PR in the next couple of days. Cheers, |
|
Wrapping the optional values in a Message wouldn't be such a big deal. It just creates clutter, but in general I don't think shortcomings of the linter and formatter should drive design decisions. |
|
I agree that the extra boilerplate is clutter, I just figured it'd be less work than trying to find a new linter/formatter :) Cheers, |
|
@ChrisCummins, did you get a chance to review this PR? |
|
Sorry @sogartar, this slipped through the net! Thanks for the nudge. I will take a look within the next 24 hours. Cheers, |
ChrisCummins
reviewed
Feb 2, 2022
There was a problem hiding this comment.
Hi @sogartar,
First, sorry for the delay in reviewing this. My schedule is mostly back to normal now so I should be more regular with responding to PRs.
Overall, I really like the direction you have taken and I think that this PR is a big improvement over the existing approach. The only merge-blocking thing I see is the need to update the CI/pre-commit configuration so that the proto linter works, or working around it by declaring 'Optional' type wrappers as discussed above.
A bunch of tiny nitpicks left inline.
I wonder if moving the name field from Space into ActionSpace.name and ObservationSpace.name would be more clear as to what the name is used for?
This is great work @sogartar, I look forward to merging it, thank you.
Cheers,
Chris
| switch (space) { | ||
| case LlvmObservationSpace::IR: { | ||
| // Serialize the LLVM module to an IR string. | ||
| std::string ir; | ||
| llvm::raw_string_ostream rso(ir); | ||
| benchmark.module().print(rso, /*AAW=*/nullptr); | ||
| rso.flush(); |
examples/example_unrolling_service/service_py/example_service.py
Outdated
Show resolved
Hide resolved
| @@ -44,4 +44,5 @@ ExternalProject_Add_Step(programl build_programl | |||
| @labm8//labm8/cpp:stringpiece | |||
| DEPENDEES update | |||
| WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/programl/src/programl" | |||
| USES_TERMINAL TRUE | |||
There was a problem hiding this comment.
Curious, what does this do?
There was a problem hiding this comment.
It will redirect the terminal output before the command has finished giving you a chance to see what is going on immediately instead of spitting it out at the end. Without this some of the long-running commands don't output anything leaving you wondering if the build is stuck.
|
BTW, I ran the micro-benchmark suite on my local machine to see what, if any, impact on performance this PR has. They're not very scientific and I expect there's a fair amount of noise in the measurements, but I thought I'd share the raw data with you here in case you're interested. For each benchmark, the results show the current development head ( The only thing that surprised me was a large regression in the "fast benchmark, fast action" step benchmark for LLVM ( |
|
One final thing, the list of protobufs in
You can use Cheers, |
|
Regarding the
I don't see a reason to keep it in |
sogartar
force-pushed
the
grpc-refactoring
branch
from
5f7014e
to
b76c7f3
Compare
|
@ChrisCummins, I added sparse documentation. It can be better. The PR is ready for a follow-up review. |
|
Hi @sogartar, thanks for addressing those comments so quickly. I think moving This is looking in good shape now. I notice that there is a couple of merge conflicts (I think they're just trivial churn in module imports). If you could please rebase off development I think this looks good to land! Cheers, |
ChrisCummins
approved these changes
Feb 3, 2022
sogartar
force-pushed
the
grpc-refactoring
branch
from
b76c7f3
to
de0cad3
Compare
|
@ChrisCummins, I squashed and rebased this PR. |
|
Fantastic, thanks so much @sogartar! Merging. Cheers, |
6 tasks
ChrisCummins
added a commit
that referenced
this pull request
May 24, 2022
This release adds a new compiler environment, new APIs, and a suite of backend improvements to improve the flexibility of CompilerGym environments. Many thanks to code contributors: @sogartar, @KyleHerndon, @SoumyajitKarmakar, @uduse, and @anthony0727! Highlights of this release include: - [mlir] Began work on a new environment for matrix multiplication using MLIR ([#652](#652), thanks @KyleHerndon and @sogartar!). Note this environment is not yet included in the pypi package and must be [compiled from source](https://github.com/facebookresearch/CompilerGym/blob/development/INSTALL.md#building-from-source-with-cmake). - [llvm] Added a new `env.benchmark_from_clang_invocation()` method ([#577](#577)) that can be used for constructing LLVM environment automatically from C/C++ compiler invocations. This makes it much easier to integrate CompilerGym with your existing build scripts. - Added three new wrapper classes: `Counter`, that provides op counts for analysis ([#683](#683)); `SynchronousSqliteLogger`, that provides logging of environment interactions to a relational database ([#679](#679)), and `ForkOnStep` that provides an `undo()` operation ([#682](#682)). - Added `reward_space` and `observation_space` parameters to `env.reset()` ([#659](#659), thanks @SoumyajitKarmakar!) This release includes a number of improvements to the backend APIs that make it easier to write new CompilerGym environments: - Refactored the backend to make `CompilerEnv` an abstract interface, and `ClientServiceCompilerEnv` the concrete implementation of this interface. This enables new environments to be implemented without using gRPC ([#633](#633), thanks @sogartar!). - Extended the support for different types of action and observation spaces ([#641](#641), [#643](#643), thanks @sogartar!), including new `Permutation` and `SpaceSequence` spaces ([#645](#645), thanks @sogartar!).. - Added a new `disk/` subdirectory to compiler service's working directories, which is symlinked to an on-disk location for devices which support in-memory working directories. This fixes a bug with leftover temporary directories from LLVM ([#672](#672)). This release also includes numerous bug fixes and improvements, many of which were reported or fixed by the community. For example, fixing a bug in cache file locations ([#656](#656), thanks @uduse!), and a missing flag definition in example code ([#684](#684), thanks @anthony0727!). **Full Changelog**: v0.2.3...v0.2.4 This release brings in deprecating changes to the core `env.step()` routine, and lays the groundwork for enabling new types of compiler optimizations to be exposed through CompilerGym. Many thanks to code contributors: @mostafaelhoushi, @sogartar, @KyleHerndon, @uduse, @parthchadha, and @xtremey! Highlights of this release include: - Added a new `TextSizeInBytes` observation space for LLVM ([#575](#575)). * Added a new PPO leaderboard entry ([#580](#580). Thanks @xtremey! - Fixed a bug in which temporary directories created by the LLVM environment were not cleaned up ([#592](#592)). - **[Backend]** The function `createAndRunCompilerGymService` now returns an int, which is the exit return code ([#592](#592)). - Improvements to the examples documentation ([#548](#548)) and FAQ ([#586](#586)) Deprecations and breaking changes: - `CompilerEnv.step` no longer accepts a list of actions ([#627](#627)). A new method, `CompilerEnv.multistep` provides this functionality. This is to provide compatibility with environments whose action spaces are lists. To update your code, replace any calls to `env.step()` which take a list of actions to use `env.multistep()`. Thanks @sogartar! - The arguments `observations` and `rewards` to `step()` have been renamed `observation_spaces` and `reward_spaces`, respectively ([#627](#627)). - `Reward.id` has been renamed `Reward.name` ([#565](#565), [#612](#612)). Thanks @parthchadha! * The backend protocol buffer schema has been updated to natively support more types of observation and action, and to support nested spaces ([#531](#531)). Thanks @sogartar!
ChrisCummins
added a commit
that referenced
this pull request
May 24, 2022
This release adds a new compiler environment, new APIs, and a suite of backend improvements to improve the flexibility of CompilerGym environments. Many thanks to code contributors: @sogartar, @KyleHerndon, @SoumyajitKarmakar, @uduse, and @anthony0727! Highlights of this release include: - [mlir] Began work on a new environment for matrix multiplication using MLIR ([#652](#652), thanks @KyleHerndon and @sogartar!). Note this environment is not yet included in the pypi package and must be [compiled from source](https://github.com/facebookresearch/CompilerGym/blob/development/INSTALL.md#building-from-source-with-cmake). - [llvm] Added a new `env.benchmark_from_clang_invocation()` method ([#577](#577)) that can be used for constructing LLVM environment automatically from C/C++ compiler invocations. This makes it much easier to integrate CompilerGym with your existing build scripts. - Added three new wrapper classes: `Counter`, that provides op counts for analysis ([#683](#683)); `SynchronousSqliteLogger`, that provides logging of environment interactions to a relational database ([#679](#679)), and `ForkOnStep` that provides an `undo()` operation ([#682](#682)). - Added `reward_space` and `observation_space` parameters to `env.reset()` ([#659](#659), thanks @SoumyajitKarmakar!) This release includes a number of improvements to the backend APIs that make it easier to write new CompilerGym environments: - Refactored the backend to make `CompilerEnv` an abstract interface, and `ClientServiceCompilerEnv` the concrete implementation of this interface. This enables new environments to be implemented without using gRPC ([#633](#633), thanks @sogartar!). - Extended the support for different types of action and observation spaces ([#641](#641), [#643](#643), thanks @sogartar!), including new `Permutation` and `SpaceSequence` spaces ([#645](#645), thanks @sogartar!).. - Added a new `disk/` subdirectory to compiler service's working directories, which is symlinked to an on-disk location for devices which support in-memory working directories. This fixes a bug with leftover temporary directories from LLVM ([#672](#672)). This release also includes numerous bug fixes and improvements, many of which were reported or fixed by the community. For example, fixing a bug in cache file locations ([#656](#656), thanks @uduse!), and a missing flag definition in example code ([#684](#684), thanks @anthony0727!). **Full Changelog**: v0.2.3...v0.2.4 This release brings in deprecating changes to the core `env.step()` routine, and lays the groundwork for enabling new types of compiler optimizations to be exposed through CompilerGym. Many thanks to code contributors: @mostafaelhoushi, @sogartar, @KyleHerndon, @uduse, @parthchadha, and @xtremey! Highlights of this release include: - Added a new `TextSizeInBytes` observation space for LLVM ([#575](#575)). * Added a new PPO leaderboard entry ([#580](#580). Thanks @xtremey! - Fixed a bug in which temporary directories created by the LLVM environment were not cleaned up ([#592](#592)). - **[Backend]** The function `createAndRunCompilerGymService` now returns an int, which is the exit return code ([#592](#592)). - Improvements to the examples documentation ([#548](#548)) and FAQ ([#586](#586)) Deprecations and breaking changes: - `CompilerEnv.step` no longer accepts a list of actions ([#627](#627)). A new method, `CompilerEnv.multistep` provides this functionality. This is to provide compatibility with environments whose action spaces are lists. To update your code, replace any calls to `env.step()` which take a list of actions to use `env.multistep()`. Thanks @sogartar! - The arguments `observations` and `rewards` to `step()` have been renamed `observation_spaces` and `reward_spaces`, respectively ([#627](#627)). - `Reward.id` has been renamed `Reward.name` ([#565](#565), [#612](#612)). Thanks @parthchadha! * The backend protocol buffer schema has been updated to natively support more types of observation and action, and to support nested spaces ([#531](#531)). Thanks @sogartar!
ChrisCummins
added a commit
that referenced
this pull request
May 24, 2022
This release adds a new compiler environment, new APIs, and a suite of backend improvements to improve the flexibility of CompilerGym environments. Many thanks to code contributors: @sogartar, @KyleHerndon, @SoumyajitKarmakar, @uduse, and @anthony0727! Highlights of this release include: - [mlir] Began work on a new environment for matrix multiplication using MLIR ([#652](#652), thanks @KyleHerndon and @sogartar!). Note this environment is not yet included in the pypi package and must be [compiled from source](https://github.com/facebookresearch/CompilerGym/blob/development/INSTALL.md#building-from-source-with-cmake). - [llvm] Added a new `env.benchmark_from_clang_invocation()` method ([#577](#577)) that can be used for constructing LLVM environment automatically from C/C++ compiler invocations. This makes it much easier to integrate CompilerGym with your existing build scripts. - Added three new wrapper classes: `Counter`, that provides op counts for analysis ([#683](#683)); `SynchronousSqliteLogger`, that provides logging of environment interactions to a relational database ([#679](#679)), and `ForkOnStep` that provides an `undo()` operation ([#682](#682)). - Added `reward_space` and `observation_space` parameters to `env.reset()` ([#659](#659), thanks @SoumyajitKarmakar!) This release includes a number of improvements to the backend APIs that make it easier to write new CompilerGym environments: - Refactored the backend to make `CompilerEnv` an abstract interface, and `ClientServiceCompilerEnv` the concrete implementation of this interface. This enables new environments to be implemented without using gRPC ([#633](#633), thanks @sogartar!). - Extended the support for different types of action and observation spaces ([#641](#641), [#643](#643), thanks @sogartar!), including new `Permutation` and `SpaceSequence` spaces ([#645](#645), thanks @sogartar!).. - Added a new `disk/` subdirectory to compiler service's working directories, which is symlinked to an on-disk location for devices which support in-memory working directories. This fixes a bug with leftover temporary directories from LLVM ([#672](#672)). This release also includes numerous bug fixes and improvements, many of which were reported or fixed by the community. For example, fixing a bug in cache file locations ([#656](#656), thanks @uduse!), and a missing flag definition in example code ([#684](#684), thanks @anthony0727!). **Full Changelog**: v0.2.3...v0.2.4 This release brings in deprecating changes to the core `env.step()` routine, and lays the groundwork for enabling new types of compiler optimizations to be exposed through CompilerGym. Many thanks to code contributors: @mostafaelhoushi, @sogartar, @KyleHerndon, @uduse, @parthchadha, and @xtremey! Highlights of this release include: - Added a new `TextSizeInBytes` observation space for LLVM ([#575](#575)). * Added a new PPO leaderboard entry ([#580](#580). Thanks @xtremey! - Fixed a bug in which temporary directories created by the LLVM environment were not cleaned up ([#592](#592)). - **[Backend]** The function `createAndRunCompilerGymService` now returns an int, which is the exit return code ([#592](#592)). - Improvements to the examples documentation ([#548](#548)) and FAQ ([#586](#586)) Deprecations and breaking changes: - `CompilerEnv.step` no longer accepts a list of actions ([#627](#627)). A new method, `CompilerEnv.multistep` provides this functionality. This is to provide compatibility with environments whose action spaces are lists. To update your code, replace any calls to `env.step()` which take a list of actions to use `env.multistep()`. Thanks @sogartar! - The arguments `observations` and `rewards` to `step()` have been renamed `observation_spaces` and `reward_spaces`, respectively ([#627](#627)). - `Reward.id` has been renamed `Reward.name` ([#565](#565), [#612](#612)). Thanks @parthchadha! * The backend protocol buffer schema has been updated to natively support more types of observation and action, and to support nested spaces ([#531](#531)). Thanks @sogartar!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #526
This is a draft of the protobuf message definitions. It makes actions and observations be more like the CompilerGym Python environment interface.
@ChrisCummins, could you take a look and see if this structure is appropriate. After I address your remarks I will proceed to refactor the existing environments to use the new structure.