Add nonnegative_int type #1533
Add nonnegative_int type #1533
Conversation
There was a problem hiding this comment.
great PR! left some small comments and fyis, nothing too important : )
Reviewed 15 of 32 files at r1, 70 of 70 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lockshaw and @victorli2002)
lib/op-attrs/test/src/op-attrs/dim_ordered/slice.cc line 7 at r2 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("slice(FFOrdered<T>, ff_dim_t, ff_dim_t)") {
Better to have a test case with 6 subcases, since this way you have to define d only once and you can then reuse it across subcases) (Or also 2 tests with 3 subcases each)
lib/op-attrs/src/op-attrs/ops/concat.cc line 50 at r2 (raw file):
} std::map<ff_dim_t, size_t> returned_wrapped; for (const auto &[key, value] : returned.value()) {
see map_keys in utils/containers/map_keys.h: )
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 342 at r2 (raw file):
std::vector<ff_dim_t> inner_to_outer_idxs(FFOrdered<T> const &ff_ordered) { std::vector<ff_dim_t> idxs; for (size_t i = 0; i < ff_ordered.size(); i++) {
alternatively, you can also use range() in 'utils/containers/range.h' (containers in general is pretty awesome, there's a lot of useful functions), though obviously like this is perfectly fine.
lib/op-attrs/include/op-attrs/relative_ff_dim_t.h line 7 at r2 (raw file):
#include "rapidcheck.h" namespace rc {
for rapidcheck support for dtgen types you can add the "rapidcheck" string in the features array (inside of the .struct.toml file). Not sure if it applies here, just an fyi : )
lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 26 at r2 (raw file):
nonnegative_int nn_int_2 = nonnegative_int{2}; SUBCASE("LHS: nonnegative_int, RHS: nonnegative_int, equal") { CHECK((nn_int_1a == nn_int_1b) == true);
to add support for doctest fmting, take a look at utils/fmt/ (this way you can do direct comparisons inside the CHECK)
lib/op-attrs/src/op-attrs/ff_dim_t.cc line 9 at r2 (raw file):
[](int value) { return FlexFlow::nonnegative_int(value); })); } } // namespace rc
see my rapidcheck comment
lib/pcg/src/pcg/computation_graph_builder.cc line 853 at r2 (raw file):
stack_vector<ff_dim_t, MAX_TENSOR_DIM> axes_stack; std::transform(axes.begin(),
betterutils/containers/transform.h (if you haven't tried it, might misbehave with stack_vector)
lib/op-attrs/include/op-attrs/dim_ordered/slice.h line 123 at r2 (raw file):
d, std::optional<relative_ff_dim_t>{start}, std::optional<relative_ff_dim_t>{end});
template
FFOrdered slice(FFOrdered const& d,
std::optional<relative_ff_dim_t> const& start = std::nullopt,
std::optional<relative_ff_dim_t> const& end = std::nullopt) {
return relative_ff_dim_t_nonoverloaded_slice(d, start, end);
}
As an alternative to group together these 3 overloads? (ditto for the other 3 overloads). If you pass a value to a function that takes in an optional it will get implictly casted (and also handles the additional case of both being std::nullopt, not sure if you'd want that or not).
Code quote:
template <typename T>
FFOrdered<T> slice(FFOrdered<T> const &d,
std::nullopt_t const &start,
relative_ff_dim_t const &end) {
return relative_ff_dim_t_nonoverloaded_slice(
d,
std::optional<relative_ff_dim_t>{start},
std::optional<relative_ff_dim_t>{end});
}
template <typename T>
FFOrdered<T> slice(FFOrdered<T> const &d,
relative_ff_dim_t const &start,
std::nullopt_t const &end) {
return relative_ff_dim_t_nonoverloaded_slice(
d,
std::optional<relative_ff_dim_t>{start},
std::optional<relative_ff_dim_t>{end});
}
template <typename T>
FFOrdered<T> slice(FFOrdered<T> const &d,
relative_ff_dim_t const &start,
relative_ff_dim_t const &end) {
return relative_ff_dim_t_nonoverloaded_slice(
d,
std::optional<relative_ff_dim_t>{start},
std::optional<relative_ff_dim_t>{end});
There was a problem hiding this comment.
Reviewed 15 of 32 files at r1, 70 of 70 files at r2, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @Marsella8 and @victorli2002)
lib/op-attrs/include/op-attrs/relative_ff_dim_t.h line 7 at r2 (raw file):
Previously, Marsella8 wrote…
for rapidcheck support for dtgen types you can add the "rapidcheck" string in the features array (inside of the .struct.toml file). Not sure if it applies here, just an fyi : )
Since ff_dim_t and relative_ff_dim_t are bounded by the MAX_TENSOR_DIM, unfortunately I don't think we can use the dtgen support as convenient as it would be.
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 342 at r2 (raw file):
Previously, Marsella8 wrote…
alternatively, you can also use range() in 'utils/containers/range.h' (containers in general is pretty awesome, there's a lot of useful functions), though obviously like this is perfectly fine.
range would definitely be good, but honestly a bunch of these index manipulation functions are super out-of-date and should probably just be removed entirely. @victorli2002 can you do a quick pass and remove any that aren't used anymore?
lib/op-attrs/include/op-attrs/dim_ordered/slice.h line 123 at r2 (raw file):
Previously, Marsella8 wrote…
template
FFOrdered slice(FFOrdered const& d,
std::optional<relative_ff_dim_t> const& start = std::nullopt,
std::optional<relative_ff_dim_t> const& end = std::nullopt) {
return relative_ff_dim_t_nonoverloaded_slice(d, start, end);
}As an alternative to group together these 3 overloads? (ditto for the other 3 overloads). If you pass a value to a function that takes in an optional it will get implictly casted (and also handles the additional case of both being std::nullopt, not sure if you'd want that or not).
IIRC we were having some issues with overload resolution functioning properly with the unified signature, though I unfortunately seem to have forgotten what the exact issue was. @victorli2002 if you want you could try changing to @Marsella8's proposed signature and see if anything breaks
lib/op-attrs/src/op-attrs/ff_dim_t.cc line 9 at r2 (raw file):
Previously, Marsella8 wrote…
see my rapidcheck comment
Link: https://reviewable.io/reviews/flexflow/FlexFlow/1533#-OBmK6xk5zBEDvogOzc_
lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 26 at r2 (raw file):
Previously, Marsella8 wrote…
to add support for doctest fmting, take a look at
utils/fmt/(this way you can do direct comparisons inside theCHECK)
IIRC all ostream-able types should automatically be printable by doctest. What error are you running into @victorli2002?
@Marsella8 is correct that this code should be refactored to CHECK(nn_int_1a == nn_int_1b)
lib/local-execution/src/legion_tensor_shape.cc line 10 at r2 (raw file):
} legion_dim_t legion_dim_from_ff_dim(ff_dim_t ff_dim, TensorShape const &shape) {
Might be better to just forward the call to legion_dim_from_ff_dim(ff_dim_t, int) in legion_dim.cc (doesn't really have to do with your changes, but while we're looking at this file)
lib/local-execution/src/ops/split.cc line 109 at r2 (raw file):
out_block_size[i], output_grad.shape, attrs.axis.value.get_value());
Might be better to instead change the signature of calc_block_size?
Code quote:
attrs.axis.value.get_value());lib/op-attrs/include/op-attrs/relative_ff_dim_t.h line 9 at r2 (raw file):
namespace rc { template <> struct Arbitrary<FlexFlow::relative_ff_dim_t> {
Not a big deal, but absolute namespaces are a bit less ambiguous
Suggestion:
struct Arbitrary<::FlexFlow::relative_ff_dim_t> {lib/op-attrs/include/op-attrs/relative_ff_dim_t.struct.toml line 10 at r2 (raw file):
"json", "fmt", ]
Suggestion:
features = [
"eq",
"ord",
"hash",
"json",
"fmt",
"rapidcheck",
]lib/utils/include/utils/nonnegative_int/nonnegative_int.h line 48 at r2 (raw file):
int value_; }; } // namespace FlexFlow
Add support for fmt (https://fmt.dev/11.0/)
Suggestion:
private:
int value_;
};
int format_as(nonnegative_int);
} // namespace FlexFlowlib/op-attrs/include/op-attrs/dim_ordered/enumerate.h line 21 at r2 (raw file):
std::map<ff_dim_t, T> enumerate(FFOrdered<T> const &ff_ordered) { std::map<ff_dim_t, T> result; for (int raw_ff_dim : count(ff_ordered.size())) {
We could probably even change count to return nonnegative_ints (doesn't have to be done now, but could be an option in the future)
Code quote:
count(ff_ordered.size())lib/pcg/src/pcg/computation_graph_builder.cc line 795 at r2 (raw file):
std::optional<std::string> const &maybe_name) { ConcatAttrs attrs = ConcatAttrs{ff_dim_t{nonnegative_int{axis}}};
We actually probably want axis to be treated as relative here (i.e., this function should take in axis, wrap it as a relative_ff_dim_t, and then do the conversion to ff_dim_t and store the ff_dim_t in attrs)
Code quote:
ConcatAttrs attrs = ConcatAttrs{ff_dim_t{nonnegative_int{axis}}};lib/pcg/src/pcg/computation_graph_builder.cc line 813 at r2 (raw file):
tensor_guid_t ComputationGraphBuilder::flat( tensor_guid_t const &input, int start_dim,
We actually probably want start_dim and end_dim to be treated as relative here
lib/pcg/src/pcg/computation_graph_builder.cc line 900 at r2 (raw file):
tensor_guid_t ComputationGraphBuilder::softmax( tensor_guid_t const &input, std::optional<int> maybe_dim,
maybe_dim here should be treated as relative (generally any dim in the ComputationGraphBuilder interface should be treated as relative)
lib/local-execution/src/ops/linear.cc line 99 at r2 (raw file):
auto attrs = acc.get_argument<LinearAttrs>(ATTRS); int in_dim = input.shape.at(ff_dim_t{nonnegative_int{0}}) + 1;
@reyna-abhyankar Why is there a + 1 here?
lib/local-execution/src/ops/linear.cc line 143 at r2 (raw file):
} int in_dim = input.shape.at(ff_dim_t{nonnegative_int{0}}) + 1;
@reyna-abhyankar Why is there a + 1 here?
Code quote:
int in_dim = input.shape.at(ff_dim_t{nonnegative_int{0}}) + 1;lib/op-attrs/include/op-attrs/parallel_tensor_dim_idx_t.variant.toml line 17 at r2 (raw file):
[[values]] type = "::FlexFlow::relative_ff_dim_t"
I'm not sure this should be relative (just style-wise/semantically). What breaks if you make it ff_dim_t?
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 157 at r2 (raw file):
template <typename T> struct DimOrdered<ff_dim_t, T> {
FYI I'm not necessarily a huge fan of doing this via template specialization, but for this PR it's fine.
lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 45 at r2 (raw file):
} TEST_CASE("nonnegative_int != comparisons") {
Some of these operator tests might(?) be cleaner with rapidcheck, e.g., you could check that <= is the same as < or ==. What you have here is fine too though
lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 201 at r2 (raw file):
} TEST_CASE("nonnegative_int adl_serializer") {
Suggestion:
TEST_CASE("adl_serializer<nonnegative_int>") {lib/op-attrs/src/op-attrs/ops/combine.cc line 32 at r2 (raw file):
ParallelTensorShape output = input; shard_dim_at_idx(output,
Might not be the worst idea to add a conversion function from ff_dim_t to relative_ff_dim_t?
lib/kernels/test/src/test_transpose_kernel.cc line 11 at r2 (raw file):
std::vector<ff_dim_t> perm = {ff_dim_t{nonnegative_int{0}}, ff_dim_t{nonnegative_int{0}}};
Shouldn't this be 1 based on the old code?
Suggestion:
ff_dim_t{nonnegative_int{1}}};
There was a problem hiding this comment.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @lockshaw and @Marsella8)
lib/kernels/test/src/test_transpose_kernel.cc line 11 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Shouldn't this be
1based on the old code?
Done.
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 342 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
rangewould definitely be good, but honestly a bunch of these index manipulation functions are super out-of-date and should probably just be removed entirely. @victorli2002 can you do a quick pass and remove any that aren't used anymore?
Done.
lib/op-attrs/include/op-attrs/dim_ordered/slice.h line 123 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
IIRC we were having some issues with overload resolution functioning properly with the unified signature, though I unfortunately seem to have forgotten what the exact issue was. @victorli2002 if you want you could try changing to @Marsella8's proposed signature and see if anything breaks
Tried it, got the following error message (which iirc was the same message I got earlier).
Code snippet:
/home/vli42/ff/nn_int/lib/op-attrs/include/op-attrs/dim_ordered/slice.h:17:21: error: could not convert 'FlexFlow::transform<FlexFlow::nonoverloaded_slice<FlexFlow::ff_dim_t, int>(const FlexFlow::DimOrdered<FlexFlow::ff_dim_t, int>&, const std::optional<FlexFlow::ff_dim_t>&, const std::optional<FlexFlow::ff_dim_t>&)::<lambda(const std::optional<FlexFlow::ff_dim_t>&)>::<lambda(const FlexFlow::ff_dim_t&)>, FlexFlow::ff_dim_t>((* & idx), <lambda closure object>FlexFlow::nonoverloaded_slice<FlexFlow::ff_dim_t, int>(const FlexFlow::DimOrdered<FlexFlow::ff_dim_t, int>&, const std::optional<FlexFlow::ff_dim_t>&, const std::optional<FlexFlow::ff_dim_t>&)::<lambda(const std::optional<FlexFlow::ff_dim_t>&)>::<lambda(const FlexFlow::ff_dim_t&)>{})' from 'optional<FlexFlow::nonnegative_int>' to 'optional<int>'
17 | return transform(idx, [](Idx const &i) { return i.value; });
| ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| optional<FlexFlow::nonnegative_int>lib/op-attrs/src/op-attrs/ops/combine.cc line 32 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Might not be the worst idea to add a conversion function from
ff_dim_ttorelative_ff_dim_t?
Done.
lib/op-attrs/src/op-attrs/ops/concat.cc line 50 at r2 (raw file):
Previously, Marsella8 wrote…
see
map_keysinutils/containers/map_keys.h: )
That map_keys takes std::unordered_map, if I'm not mistaken. Is there a good way to use it with std::map?
lib/op-attrs/test/src/op-attrs/dim_ordered/slice.cc line 7 at r2 (raw file):
Previously, Marsella8 wrote…
Better to have a test case with 6 subcases, since this way you have to define d only once and you can then reuse it across subcases) (Or also 2 tests with 3 subcases each)
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 795 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
We actually probably want
axisto be treated as relative here (i.e., this function should take inaxis, wrap it as arelative_ff_dim_t, and then do the conversion toff_dim_tand store theff_dim_tinattrs)
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 813 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
We actually probably want
start_dimandend_dimto be treated as relative here
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 853 at r2 (raw file):
Previously, Marsella8 wrote…
better
utils/containers/transform.h(if you haven't tried it, might misbehave with stack_vector)
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 900 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
maybe_dimhere should be treated as relative (generally any dim in theComputationGraphBuilderinterface should be treated asrelative)
Done.
lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 26 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
IIRC all ostream-able types should automatically be printable by doctest. What error are you running into @victorli2002?
@Marsella8 is correct that this code should be refactored to
CHECK(nn_int_1a == nn_int_1b)
Done.
lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 201 at r2 (raw file):
} TEST_CASE("nonnegative_int adl_serializer") {
Done.
lib/op-attrs/include/op-attrs/relative_ff_dim_t.struct.toml line 10 at r2 (raw file):
"json", "fmt", ]
The original doesn't have ff_dim.struct.toml; why do we need it here?
There was a problem hiding this comment.
Reviewable status: 67 of 88 files reviewed, 19 unresolved discussions (waiting on @lockshaw and @Marsella8)
lib/op-attrs/include/op-attrs/parallel_tensor_dim_idx_t.variant.toml line 17 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
I'm not sure this should be
relative(just style-wise/semantically). What breaks if you make itff_dim_t?
Done.
lib/utils/include/utils/nonnegative_int/nonnegative_int.h line 48 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Add support for
fmt(https://fmt.dev/11.0/)
Done.
There was a problem hiding this comment.
Reviewed 21 of 21 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @Marsella8 and @victorli2002)
lib/op-attrs/include/op-attrs/relative_ff_dim_t.struct.toml line 10 at r2 (raw file):
Previously, victorli2002 (Victor Li) wrote…
The original doesn't have ff_dim.struct.toml; why do we need it here?
True, this shouldn't be here due to https://reviewable.io/reviews/flexflow/flexflow-train/1533#-OBmK6xk5zBEDvogOzc_
lib/op-attrs/include/op-attrs/dim_ordered/slice.h line 123 at r2 (raw file):
Previously, victorli2002 (Victor Li) wrote…
Tried it, got the following error message (which iirc was the same message I got earlier).
That type error seems unsurprising: line 17 declares that it will return a std::optional<int>, but instead of returning i.value.get_value() you return i.value, which would result in returning a std::optional<nonnegative_int>. There very well may be a type issue, but this doesn't seem like one of them.
I'd recommend using utils/archetypes/value_type.h to force these overloads to be typechecked, e.g., https://github.com/flexflow/FlexFlow/blob/89c143d33b9e02faf8d6818a9c47b0d5da68a610/lib/utils/src/utils/containers/get_all_assignments.cc#L6-L10
lib/op-attrs/src/op-attrs/ops/concat.cc line 50 at r2 (raw file):
Previously, victorli2002 (Victor Li) wrote…
That map_keys takes std::unordered_map, if I'm not mistaken. Is there a good way to use it with std::map?
Add an overload for std::map--we often just materialize the overloads lazily as needed rather than trying to get them all up-front
lib/op-attrs/test/src/op-attrs/dim_ordered/slice.cc line 7 at r2 (raw file):
Previously, victorli2002 (Victor Li) wrote…
Done.
In general we prefer one test-case per function, but in cases like this where there's a huge degree of overlap combining them can be a good idea
lib/pcg/src/pcg/computation_graph_builder.cc line 853 at r2 (raw file):
Previously, victorli2002 (Victor Li) wrote…
Done.
I'm still seeing std::transform?
lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 7 at r4 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("FF_DIM_T_TO_RELATIVE_FF_DIM_T") {
Remove all caps?
Code quote:
TEST_CASE("FF_DIM_T_TO_RELATIVE_FF_DIM_T") {lib/op-attrs/include/op-attrs/relative_ff_dim_t.h line 9 at r4 (raw file):
namespace FlexFlow { ff_dim_t relative_ff_dim_t_to_ff_dim_t(relative_ff_dim_t ff_dim, int input_dim);
Could make this a nonnegative_int if you wanted
Code quote:
int input_dim)lib/op-attrs/test/src/op-attrs/dim_ordered/slice.cc line 7 at r4 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("slice(FFOrdered<T>, ..., ...") {
Test error cases (e.g., out-of-bounds behavior, start being > stop, etc.)
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 7 at r4 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("RELATIVE_FF_DIM_T_TO_FF_DIM_T") {
Remove all caps?
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 34 at r4 (raw file):
relative_ff_dim_t relative_ff_dim = relative_ff_dim_t{-10}; CHECK_THROWS(relative_ff_dim_t_to_ff_dim_t(relative_ff_dim, input_dim)); }
Also check the other out of range (i.e., too high)
Code quote:
SUBCASE("OUT OF RANGE") {
relative_ff_dim_t relative_ff_dim = relative_ff_dim_t{-10};
CHECK_THROWS(relative_ff_dim_t_to_ff_dim_t(relative_ff_dim, input_dim));
}lib/op-attrs/src/op-attrs/relative_ff_dim_t.cc line 5 at r4 (raw file):
namespace FlexFlow { ff_dim_t relative_ff_dim_t_to_ff_dim_t(relative_ff_dim_t ff_dim,
What's the expected behavior if ff_dim.value > input_dim? I'm fine with the behavior in the current code below, just make sure it's tested
lib/op-attrs/src/op-attrs/ops/layer_norm.cc line 169 at r4 (raw file):
transform(non_layer_norm_dim_idxs, [&](ff_dim_t const &dim_idx) { return shard_dim_at_idx(input_shape, ff_dim_t_to_relative_ff_dim_t(dim_idx));
I'd prefer relative_ff_dim_t_from_ff_dim_t as the sides of the name match up nicer (i.e., the argument type is next to where the argument actually is) (and usually same for other conversion functions)
Code quote:
ff_dim_t_to_relative_ff_dim_tlib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 335 at r4 (raw file):
template <typename T> std::vector<ff_dim_t> inner_to_outer_idxs(FFOrdered<T> const &ff_ordered) {
Out of curiosity, where is this used?
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 337 at r4 (raw file):
std::vector<ff_dim_t> inner_to_outer_idxs(FFOrdered<T> const &ff_ordered) { std::vector<ff_dim_t> idxs; for (int i : range(0, ff_ordered.size())) {
Suggestion:
for (int i : range(ff_ordered.size())) {lib/op-attrs/src/op-attrs/parallel_tensor_shape.cc line 144 at r4 (raw file):
std::unordered_set<parallel_tensor_dim_idx_t> indices; extend(indices, transform(range(num_shard_dims(shape.dims)), [](int idx) { return parallel_tensor_dim_idx_t(ff_dim_t{nonnegative_int{idx}});
Prefer brace initialization
Suggestion:
return parallel_tensor_dim_idx_t{ff_dim_t{nonnegative_int{idx}}};lib/op-attrs/src/op-attrs/parallel_tensor_shape.cc line 146 at r4 (raw file):
return parallel_tensor_dim_idx_t(ff_dim_t{nonnegative_int{idx}}); })); indices.insert(parallel_tensor_dim_idx_t(ReplicaType::SUM));
Suggestion:
indices.insert(parallel_tensor_dim_idx_t{ReplicaType::SUM});lib/op-attrs/src/op-attrs/parallel_tensor_shape.cc line 147 at r4 (raw file):
})); indices.insert(parallel_tensor_dim_idx_t(ReplicaType::SUM)); indices.insert(parallel_tensor_dim_idx_t(ReplicaType::DISCARD_COPY));
Suggestion:
indices.insert(parallel_tensor_dim_idx_t{ReplicaType::DISCARD_COPY});lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 250 at r4 (raw file):
} TEST_CASE("nonnegative int fmt::to_string") {
Suggestion:
TEST_CASE("fmt::to_string(nonnegative_int)") {
There was a problem hiding this comment.
Reviewable status: 72 of 88 files reviewed, 20 unresolved discussions (waiting on @lockshaw and @Marsella8)
lib/op-attrs/include/op-attrs/relative_ff_dim_t.h line 9 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Could make this a
nonnegative_intif you wanted
I think this change would fight better in the later PR where I put nonnegative_int in more places.
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 335 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Out of curiosity, where is this used?
it's in local-execution/src/ops/transpose.cc
lib/op-attrs/src/op-attrs/parallel_tensor_shape.cc line 144 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Prefer brace initialization
Done.
lib/op-attrs/src/op-attrs/relative_ff_dim_t.cc line 5 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
What's the expected behavior if
ff_dim.value>input_dim? I'm fine with the behavior in the current code below, just make sure it's tested
Added a test; currently it just makes a ff_dim_t with the same value as ff_dim.value.
lib/op-attrs/src/op-attrs/ops/concat.cc line 50 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Add an overload for
std::map--we often just materialize the overloads lazily as needed rather than trying to get them all up-front
Done.
lib/op-attrs/src/op-attrs/ops/layer_norm.cc line 169 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
I'd prefer
relative_ff_dim_t_from_ff_dim_tas the sides of the name match up nicer (i.e., the argument type is next to where the argument actually is) (and usually same for other conversion functions)
Done.
lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 7 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Remove all caps?
Done.
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 7 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Remove all caps?
Done.
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 34 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Also check the other out of range (i.e., too high)
Done.
lib/op-attrs/test/src/op-attrs/dim_ordered/slice.cc line 7 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Test error cases (e.g., out-of-bounds behavior, start being > stop, etc.)
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 853 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
I'm still seeing
std::transform?
Done.
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 337 at r4 (raw file):
std::vector<ff_dim_t> inner_to_outer_idxs(FFOrdered<T> const &ff_ordered) { std::vector<ff_dim_t> idxs; for (int i : range(0, ff_ordered.size())) {
Done.
lib/utils/test/src/utils/nonnegative_int/nonnegative_int.cc line 250 at r4 (raw file):
} TEST_CASE("nonnegative int fmt::to_string") {
Done.
lib/op-attrs/src/op-attrs/parallel_tensor_shape.cc line 146 at r4 (raw file):
return parallel_tensor_dim_idx_t(ff_dim_t{nonnegative_int{idx}}); })); indices.insert(parallel_tensor_dim_idx_t(ReplicaType::SUM));
Done.
lib/op-attrs/src/op-attrs/parallel_tensor_shape.cc line 147 at r4 (raw file):
})); indices.insert(parallel_tensor_dim_idx_t(ReplicaType::SUM)); indices.insert(parallel_tensor_dim_idx_t(ReplicaType::DISCARD_COPY));
Done.
There was a problem hiding this comment.
Reviewable status: 71 of 88 files reviewed, 20 unresolved discussions (waiting on @lockshaw and @Marsella8)
lib/op-attrs/include/op-attrs/dim_ordered/slice.h line 123 at r2 (raw file):
Previously, lockshaw (Colin Unger) wrote…
That type error seems unsurprising: line 17 declares that it will return a
std::optional<int>, but instead of returningi.value.get_value()you returni.value, which would result in returning astd::optional<nonnegative_int>. There very well may be a type issue, but this doesn't seem like one of them.I'd recommend using
utils/archetypes/value_type.hto force these overloads to be typechecked, e.g., https://github.com/flexflow/FlexFlow/blob/89c143d33b9e02faf8d6818a9c47b0d5da68a610/lib/utils/src/utils/containers/get_all_assignments.cc#L6-L10
Done.
There was a problem hiding this comment.
Reviewed 16 of 16 files at r5, 1 of 1 files at r7, all commit messages.
Dismissed @Marsella8 from 8 discussions.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @victorli2002)
lib/op-attrs/include/op-attrs/dim_ordered/dim_ordered.h line 335 at r4 (raw file):
Previously, victorli2002 (Victor Li) wrote…
it's in local-execution/src/ops/transpose.cc
If that's the only place, I'd just inline this function there and then remove the definition here--I don't think this is an operation that necessarily makes a whole lot of sense to have on FFOrdered
lib/op-attrs/include/op-attrs/dim_ordered/slice.h line 13 at r7 (raw file):
namespace FlexFlow { using T = value_type<0>;
Should be done in the slice.cc file, not in the .h file
Code quote:
using T = value_type<0>;lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 7 at r7 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("ff_dim_to_to_relative_ff_dim_t") {
Suggestion:
TEST_CASE("relative_ff_dim_t_from_ff_dim_t") {lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 8 at r7 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("ff_dim_to_to_relative_ff_dim_t") { SUBCASE("Zero") {
Suggestion:
SUBCASE("absolute index is zero") {lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 15 at r7 (raw file):
} SUBCASE("Positive") {
Suggestion:
SUBCASE("absolute index is positive") {lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 10 at r7 (raw file):
int input_dim = 5; SUBCASE("Zero") {
Suggestion:
SUBCASE("relative index is zero") {lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 13 at r7 (raw file):
relative_ff_dim_t relative_ff_dim = relative_ff_dim_t{0}; ff_dim_t ff_dim = ff_dim_t_from_relative_ff_dim_t_ff_dim_t(relative_ff_dim, input_dim);
Suggestion:
ff_dim_t_from_relative_ff_dim_tlib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 17 at r7 (raw file):
} SUBCASE("Positive") {
Suggestion:
SUBCASE("relative index is positive") {lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 24 at r7 (raw file):
} SUBCASE("Negative") {
Suggestion:
SUBCASE("relative index is negative") {lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 31 at r7 (raw file):
} SUBCASE("Negative out of range") {
Ideally two nested subcases under "relative index is negative"--one where it is in range and one where it is out of range
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 42 at r7 (raw file):
ff_dim_t_from_relative_ff_dim_t_ff_dim_t(relative_ff_dim, input_dim); CHECK(ff_dim == ff_dim_t{nonnegative_int{10}}); }
Ideally two nested subcases under "relative index is positive"--one where it is in range, and one where it is out of range
Code quote:
SUBCASE("Positive out of range") {
relative_ff_dim_t relative_ff_dim = relative_ff_dim_t{10};
ff_dim_t ff_dim =
ff_dim_t_from_relative_ff_dim_t_ff_dim_t(relative_ff_dim, input_dim);
CHECK(ff_dim == ff_dim_t{nonnegative_int{10}});
}lib/op-attrs/test/src/op-attrs/dim_ordered/slice.cc line 68 at r7 (raw file):
CHECK(result == correct); } SUBCASE("start index > stop index") {
Is there a reason we don't throw in this case?
lib/op-attrs/src/op-attrs/ff_dim_t.cc line 13 at r7 (raw file):
return gen::construct<::FlexFlow::ff_dim_t>( gen::map(gen::inRange<int>(0, MAX_TENSOR_DIM), [](int value) { return FlexFlow::nonnegative_int(value); }));
Suggestion:
[](int value) { return FlexFlow::nonnegative_int{value}; }));lib/pcg/src/pcg/computation_graph_builder.cc line 865 at r7 (raw file):
std::back_inserter(axes_stack), [&input_shape](int axis) { return ff_dim_t_from_relative_ff_dim_t_ff_dim_t(
Normalization should be done before the bounds-checking on axes, not afterward
lib/pcg/src/pcg/computation_graph_builder.cc line 867 at r7 (raw file):
return ff_dim_t_from_relative_ff_dim_t_ff_dim_t( relative_ff_dim_t{axis}, num_dims(input_shape)); });
Suggestion:
stack_vector<ff_dim_t, MAX_TENSOR_DIM> axes_stack =
transform(axes, [&](int axis) {
return ff_dim_t_from_relative_ff_dim_t_ff_dim_t(
relative_ff_dim_t{axis}, num_dims(input_shape));
});lib/pcg/src/pcg/computation_graph_builder.cc line 926 at r7 (raw file):
} SoftmaxAttrs attrs = SoftmaxAttrs{ff_dim_t_from_relative_ff_dim_t_ff_dim_t(
Normalization should be ideally done before the bounds checking on dim
Code quote:
ff_dim_t_from_relative_ff_dim_t_ff_dim_t(
There was a problem hiding this comment.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @lockshaw)
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 31 at r7 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Ideally two nested subcases under "relative index is negative"--one where it is in range and one where it is out of range
Done.
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 42 at r7 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Ideally two nested subcases under "relative index is positive"--one where it is in range, and one where it is out of range
Done.
lib/op-attrs/test/src/op-attrs/dim_ordered/slice.cc line 68 at r7 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Is there a reason we don't throw in this case?
That's just how the behavior works atm, can fix if needed. (It uses the subvec function)
lib/pcg/src/pcg/computation_graph_builder.cc line 865 at r7 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Normalization should be done before the bounds-checking on
axes, not afterward
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 926 at r7 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Normalization should be ideally done before the bounds checking on
dim
Done.
lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 7 at r7 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("ff_dim_to_to_relative_ff_dim_t") {
Done.
lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 8 at r7 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("ff_dim_to_to_relative_ff_dim_t") { SUBCASE("Zero") {
Done.
lib/op-attrs/test/src/op-attrs/ff_dim_t.cc line 15 at r7 (raw file):
} SUBCASE("Positive") {
Done.
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 10 at r7 (raw file):
int input_dim = 5; SUBCASE("Zero") {
Done.
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 13 at r7 (raw file):
relative_ff_dim_t relative_ff_dim = relative_ff_dim_t{0}; ff_dim_t ff_dim = ff_dim_t_from_relative_ff_dim_t_ff_dim_t(relative_ff_dim, input_dim);
Done.
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 17 at r7 (raw file):
} SUBCASE("Positive") {
Done.
lib/op-attrs/test/src/op-attrs/relative_ff_dim_t.cc line 24 at r7 (raw file):
} SUBCASE("Negative") {
Done.
lib/op-attrs/src/op-attrs/ff_dim_t.cc line 13 at r7 (raw file):
return gen::construct<::FlexFlow::ff_dim_t>( gen::map(gen::inRange<int>(0, MAX_TENSOR_DIM), [](int value) { return FlexFlow::nonnegative_int(value); }));
Done.
There was a problem hiding this comment.
Reviewed 28 of 28 files at r8, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @victorli2002)
lib/utils/src/utils/stack_vector/stack_vector_of.cc line 1 at r8 (raw file):
#include "utils/stack_vector/stack_vector_of.h"
Add explicit instantiation of stack_vector_of for stack_vector<value_type, ...>
lib/local-execution/src/ops/transpose.cc line 43 at r8 (raw file):
auto const &attrs = acc.get_argument<TransposeAttrs>(ATTRS); std::vector<ff_dim_t> perm; size_t const &size = attrs.perm.size();
Don't use variables with reference type unless you're certain they're useful (which in this case it's not, the pointer is probably no smaller than the size_t anyway)
Suggestion:
size_t size = attrs.perm.size();lib/local-execution/src/ops/transpose.cc line 46 at r8 (raw file):
for (size_t const i : range(size)) { perm.push_back(ff_dim_t{nonnegative_int{size - i - 1}}); }
Avoid unnecessary const
Suggestion:
for (size_t i : range(size)) {
perm.push_back(ff_dim_t{nonnegative_int{size - i - 1}});
}lib/local-execution/src/ops/transpose.cc line 46 at r8 (raw file):
for (size_t const i : range(size)) { perm.push_back(ff_dim_t{nonnegative_int{size - i - 1}}); }
Prefer performing multi-step initialization in its own block unless it's essentially the only thing in the current block
Suggestion:
std::vector<ff_dim_t> perm = [&] {
std::vector<ff_dim_t> result;
size_t size = attrs.perm.size();
for (size_t const i : range(size)) {
result.push_back(ff_dim_t{nonnegative_int{size - i - 1}});
}
return result;
}();lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
#include "utils/stack_vector/stack_vector.h"
Add explicit instantiation of stack_vector<value_type, ...>
lib/op-attrs/src/op-attrs/dim_ordered/slice.cc line 11 at r8 (raw file):
using T = value_type<0>; } // namespace FlexFlow
You actually have to instantiate the template functions (the using T is just declaring a type alias)
Suggestion:
namespace FlexFlow {
using T = value_type<0>;
template
FFOrdered<T> ff_dim_t_nonoverloaded_slice(FFOrdered<T> const &d,
std::optional<ff_dim_t> const &start,
std::optional<ff_dim_t> const &end);
template
FFOrdered<T> relative_ff_dim_t_nonoverloaded_slice(
FFOrdered<T> const &d,
std::optional<relative_ff_dim_t> const &start,
std::optional<relative_ff_dim_t> const &end);
template
FFOrdered<T> slice(FFOrdered<T> const &d,
std::optional<ff_dim_t> const &start,
std::optional<ff_dim_t> const &end);
template
FFOrdered<T> slice(FFOrdered<T> const &d,
std::optional<relative_ff_dim_t> const &start,
std::optional<relative_ff_dim_t> const &end);
} // namespace FlexFlowlib/pcg/src/pcg/computation_graph_builder.cc line 860 at r8 (raw file):
if (any_of(axes_stack, [&](ff_dim_t axis) { return axis.value >= num_dims(input_shape); })) {
Suggestion:
auto resolve_dim_idx = [&](int dim_idx) {
return ff_dim_t_from_relative_ff_dim_t(relative_ff_dim_t{axis},
num_dims(input_shape));
};
stack_vector<ff_dim_t, MAX_TENSOR_DIM> axes_stack =
stack_vector_of<MAX_TENSOR_DIM>(transform(axes, resolve_dim_idx));
if (any_of(axes_stack, [&](ff_dim_t axis) {
return axis.value >= num_dims(input_shape);
})) {lib/pcg/src/pcg/computation_graph_builder.cc line 860 at r8 (raw file):
if (any_of(axes_stack, [&](ff_dim_t axis) { return axis.value >= num_dims(input_shape); })) {
Suggestion:
tensor_guid_t ComputationGraphBuilder::layer_norm(
tensor_guid_t const &input,
std::vector<int> const &relative_axes,
bool elementwise_affine,
float eps,
std::optional<std::string> const &maybe_name) {
TensorShape input_shape = this->get_shape(input);
stack_vector<ff_dim_t, MAX_TENSOR_DIM> axes =
stack_vector_of<MAX_TENSOR_DIM>(transform(relative_axes, [&](int axis) {
return ff_dim_t_from_relative_ff_dim_t(relative_ff_dim_t{axis},
num_dims(input_shape));
}));
if (any_of(axes, [&](ff_dim_t axis) {
return axis.value >= num_dims(input_shape);
})) {lib/utils/include/utils/stack_vector/stack_vector_of.h line 8 at r8 (raw file):
namespace FlexFlow { template <size_t max_size, typename C, typename E = typename C::value_type>
We generally have template params in ALL_CAPS
Suggestion:
template <size_t MAX_SIZE, typename C, typename E = typename C::value_type>lib/utils/test/src/utils/stack_vector/stack_vector_of.cc line 15 at r8 (raw file):
CHECK(result[0] == 1); CHECK(result[1] == 2); CHECK(result[2] == 3);
Prefer .at for bounds-checking
Suggestion:
CHECK(result.at(0) == 1);
CHECK(result.at(1) == 2);
CHECK(result.at(2) == 3);lib/utils/test/src/utils/stack_vector/stack_vector_of.cc line 15 at r8 (raw file):
CHECK(result[0] == 1); CHECK(result[1] == 2); CHECK(result[2] == 3);
Suggestion:
stack_vector<int, MAXSIZE> result = stack_vector_of<MAXSIZE>(input);
stack_vector<int, MAXSIZE> correct = {1, 2, 3};
CHECK(result == correct);
There was a problem hiding this comment.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @lockshaw)
lib/local-execution/src/ops/transpose.cc line 43 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Don't use variables with reference type unless you're certain they're useful (which in this case it's not, the pointer is probably no smaller than the size_t anyway)
Done.
lib/local-execution/src/ops/transpose.cc line 46 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Avoid unnecessary
const
Done.
lib/local-execution/src/ops/transpose.cc line 46 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Prefer performing multi-step initialization in its own block unless it's essentially the only thing in the current block
Done.
lib/op-attrs/src/op-attrs/dim_ordered/slice.cc line 11 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
You actually have to instantiate the template functions (the
using Tis just declaring a type alias)
Getting these errors
lib/utils/include/utils/stack_vector/stack_vector_of.h line 8 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
We generally have template params in ALL_CAPS
Done.
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Add explicit instantiation of
stack_vector<value_type, ...>
What do you mean explicit instantiation?
lib/utils/test/src/utils/stack_vector/stack_vector_of.cc line 15 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Prefer
.atfor bounds-checking
Done.
lib/utils/test/src/utils/stack_vector/stack_vector_of.cc line 15 at r8 (raw file):
CHECK(result[0] == 1); CHECK(result[1] == 2); CHECK(result[2] == 3);
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 860 at r8 (raw file):
if (any_of(axes_stack, [&](ff_dim_t axis) { return axis.value >= num_dims(input_shape); })) {
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 860 at r8 (raw file):
if (any_of(axes_stack, [&](ff_dim_t axis) { return axis.value >= num_dims(input_shape); })) {
Done.
lib/pcg/src/pcg/computation_graph_builder.cc line 867 at r7 (raw file):
return ff_dim_t_from_relative_ff_dim_t_ff_dim_t( relative_ff_dim_t{axis}, num_dims(input_shape)); });
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @victorli2002)
lib/op-attrs/src/op-attrs/dim_ordered/slice.cc line 11 at r8 (raw file):
Previously, victorli2002 (Victor Li) wrote…
Getting these errors
I think the issue here is the ifndefs in this file--you should only have header guards in headers
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, victorli2002 (Victor Li) wrote…
What do you mean explicit instantiation?
using T = value_type<0>;
template struct stack_vector<T, 5>;
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lockshaw)
lib/op-attrs/src/op-attrs/dim_ordered/slice.cc line 11 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
I think the issue here is the
ifndefs in this file--you should only have header guards in headers
oops lol
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
using T = value_type<0>; template struct stack_vector<T, 5>;
value_type looks like it's missing some operators that are used in stack_vector
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @victorli2002)
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, victorli2002 (Victor Li) wrote…
value_type looks like it's missing some operators that are used in stack_vector
In that case try using ordered_value_type instead
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lockshaw)
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
In that case try using
ordered_value_typeinstead
I can't find that type. Is it in the main branch?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @victorli2002)
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, victorli2002 (Victor Li) wrote…
I can't find that type. Is it in the main branch?
Good point, go ahead and copy it into this branch: implementation here
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lockshaw)
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Good point, go ahead and copy it into this branch: implementation here
Is there a way to give it support for std::optional?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @victorli2002)
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, victorli2002 (Victor Li) wrote…
Is there a way to give it support for std::optional?
That's actually a correctly-detected error: ordered_value_type is not default-constructible, so element_type is std::optional<ordered_value_type>, but then the type of the .data() method is incorrect. For now just remove that method
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lockshaw)
lib/utils/src/utils/stack_vector/stack_vector.cc line 1 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
That's actually a correctly-detected error:
ordered_value_typeis not default-constructible, soelement_typeisstd::optional<ordered_value_type>, but then the type of the.data()method is incorrect. For now just remove that method
Done.
lib/utils/src/utils/stack_vector/stack_vector_of.cc line 1 at r8 (raw file):
Previously, lockshaw (Colin Unger) wrote…
Add explicit instantiation of
stack_vector_offorstack_vector<value_type, ...>
Done.
There was a problem hiding this comment.
Reviewable status: 99 of 106 files reviewed, 6 unresolved discussions (waiting on @lockshaw and @Marsella8)
lib/utils/include/utils/stack_vector/stack_vector.h line 295 at r10 (raw file):
} std::array<element_type, MAXSIZE> stack_contents() const {
Changed this due to the type errors with ordered_value_type. stack_string.h still called the data function, so I created this to give it access to contents
lib/utils/include/utils/stack_string.h line 62 at r10 (raw file):
friend fmt::basic_string_view<Char> format_as(stack_basic_string<Char, MAXSIZE> const &s) { return {s.contents.stack_contents().data(), s.length()};
changed to match stack_vector change
lockshaw
changed the title
There was a problem hiding this comment.
Reviewed 5 of 7 files at r10, 5 of 5 files at r11, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Marsella8 and @victorli2002)
Description of changes:
This change is