New Smesher API #145

moshababo · 2021-05-24T04:05:51Z

Main remarks:

Most methods which were directly dealing with the PoST setup were removed.
Changed IsSmeshing() -> bool with SmeshingStatus() -> [IDLE, CREATING_POST_DATA, ACTIVE]. Since Smeshing now triggers the PoST setup, we have an intermediate stage in which we are neither idle or Smeshing (NOTE: ACTIVE is probably a bad name because it has nothing to do with the activation status of the miner).
As previously discussed, PostStatus() query is no longer supported (because a status can be evaluated only in relation to aPostInitOpts arg), besides within PostDataCreationProgressStream, which is supported only when a PoST data creation session is active. For this reason I changed the status message name to SessionStatus and removed most of its fields (files_status, init_in_progress, error_type, error_message), while keeping only num_labels_written (instead of bytes_written). We can rethink this and decide whether we want to expose errors within the stream or elsewhere, asynchronously.
Within PostInitOpts the user is now to select its commitment size via num_units instead of data_size. For evaluating the size in bytes, clients are expected to query Config() which specifies labels_per_unit and bits_per_label.
PostComputeProviders() now have bool benchmark, to avoid imposing the benchmark procedure delay by default (for example when a client wants to enumerate the providers for ID validation purposes).

# Conflicts: # proto/spacemesh/v1/smesher.proto # release/go/spacemesh/v1/smesher.pb.go # release/go/spacemesh/v1/smesher_types.pb.go

proto/spacemesh/v1/smesher_types.proto

moshababo · 2021-06-05T15:12:47Z

Changes:

Returned SmeshingStatus() -> [IDLE, CREATING_POST_DATA, ACTIVE] to IsSmeshing() -> bool.
It’s referring solely to the phase in which post setup completed and ATX creation started.
Returned PostStatus() on a limited-scope. If setup started, it’s referring to the current/latest PostInitOpts, otherwise to none. Its response is identical to PostDataCreationProgressStream's.
The error in PostStatus() is not mutually-exclusive with other fields, so I did not use the Oneof protobuf pattern.
It can be made mutually-exclusive in PostDataCreationProgressStream, but I thought it’s not necessary, and keeping it identical to PostStatus()'s response is preferred.

We need to conclude the terminology dicsussion so I could finalize the documentation and how we name things. Currently, “pos”, “pos setup”, “post data creation”, “post init”, “post setup” are used interchangeably. I vote for the later.

avive · 2021-06-05T16:21:03Z

We need to conclude the terminology dicsussion so I could finalize the documentation and how we name things. Currently, “pos”, “pos setup”, “post data creation”, “post init”, “post setup” are used interchangeably. I vote for the later.

For now I see you call its Post and not Pos on the API level. We can use post setup in stead of pos setup on the ui level as well to make you and tal happy.

avive · 2021-06-05T16:23:04Z

Changes:

Q: are you updating cli-wallet to work with these changes as well?

avive · 2021-06-05T16:29:02Z

Can you please remove the status field from all smesher api responses? They are not needed as a grcp api response always has built-in status code and message fields.

avive · 2021-06-05T16:34:23Z

Changes:

Returned SmeshingStatus() -> [IDLE, CREATING_POST_DATA, ACTIVE] to IsSmeshing() -> bool.
It’s referring solely to the phase in which post setup completed and ATX creation started.

Returned PostStatus() on a limited-scope. If setup started, it’s referring to the current/latest PostInitOpts, otherwise to none. Its response is identical to PostDataCreationProgressStream's.

LGTM but it will be nice to add these notes to the doc comments of the methods in smesher.proto so the intent is clearer there for all users and this will be used as base for higher-level api docs.

moshababo · 2021-06-05T17:15:16Z

Q: are you updating cli-wallet to work with these changes as well?

Updated here: spacemeshos/smrepl#23 (comment).

Can you please remove the status field from all smesher api responses? They are not needed as a grcp api response always has built-in status code and message fields.

This pattern is used in other services as well, so I'd rather keep it following the general API design, and discuss it with @lrettig.

LGTM but it will be nice to add these notes to the doc comments of the methods in smesher.proto so the intent is clearer there for all users and this will be used as base for higher-level api docs.

I planned to finish the docs once the design and the implementation is accepted.

lrettig · 2021-06-12T19:58:27Z

proto/spacemesh/v1/smesher.proto

+    rpc IsSmeshing (google.protobuf.Empty) returns (IsSmeshingResponse);
+
+    // Start smeshing. If the post data is incomplete or missing, data creation
+    // session will be preceded. Changing of the post potions (e.g., number of labels),


"will be preceded"

What does this mean? is it a typo?

It means that if the data is incomplete or missing, data creation session will occur and must complete before smeshing can start. Feel free to fix my english, but I plan to revise all the documentation anyway.

lrettig · 2021-06-12T20:00:02Z

proto/spacemesh/v1/smesher_types.proto

+    uint32 num_units = 2; // Number of PoST data commitment units to generate
+    uint32 num_files = 3; // Number of files to equally distribute the labels among
+    uint32 compute_provider_id = 4; // A PostProvider id
+    bool throttle = 5; // Throttle down setup phase computations while user is interactive on system


Couldn't this be an int, to allow more fine-tuned throttling? E.g., max CPU percentage?

It's referring to the utilization of GPU cores, not CPU time. The API complies with the internal API of gpu-post lib, so we should consider changing it there first.

For the release of gpu-post lib for post it is indeed a boolean and it is not going to change to an int. Let's consider this feature for next release. We need to finalize and ship and we shouldn't change features so late in the cycle.

lrettig · 2021-06-12T20:00:19Z

proto/spacemesh/v1/smesher_types.proto

-message StopPostDataCreationSessionRequest {
-    bool delete_files = 1;
+message PostComputeProvidersRequest {
+    bool benchmark = 1;


What does benchmark mean here?

Request to run a short benchmarking session for each provider, so that the performance field of PoSTSetupComputeProvider will be assigned with the num of hashes per sec.

Documentation is still missing and will be complete soon.

lrettig · 2021-06-12T20:01:45Z

Can you please remove the status field from all smesher api responses? They are not needed as a grcp api response always has built-in status code and message fields.

This pattern is used in other services as well, so I'd rather keep it following the general API design, and discuss it with @lrettig.

We use this pattern for responses that would otherwise be empty. It's a way to explicitly specify success/failure.

avive · 2021-06-13T06:44:56Z

Can you please remove the status field from all smesher api responses? They are not needed as a grcp api response always has built-in status code and message fields.

This pattern is used in other services as well, so I'd rather keep it following the general API design, and discuss it with @lrettig.

We use this pattern for responses that would otherwise be empty. It's a way to explicitly specify success/failure.

@lrettig - didn't you just comment on another issue that we should get rid of these statuses in empty responses? There you agreed this is unnecessary. In the current pattern - 2 distinct statuses are returned for these methods - the built in one (which is always returned) and the explicit one - not a great way to return results to api users.

lrettig · 2021-06-18T20:02:01Z

Yes, I agree, I think we should make this change

avive · 2021-06-27T08:10:22Z

The smesher api generally looks fine.
I have 2 relatively minor comments.

there's a bit of duplication that can go away between PostInitOpts and ConfigResponse data items. Consider having a shareג object for post config params that is returned in ConfigResponse and is also passed by caller of CreatePostDataRequest. Something like this:

message PostParams {
    uint32 bits_per_label = 1;
    uint64 labels_per_unit = 2;
    uint32 min_num_units = 3;
    uint32 max_num_units = 4;
}

message ConfigResponse {
    PostParams post_params = 1;
}

message PostInitOpts {
    PostPramas post_params = 1;
    uint32 compute_provider_id = 2; // A PostProvider id
    bool throttle = 3; // Throttle down setup phase computations while user is interactive on system
}

Also, perhaps the computer_provider_id and throttle should be included in the post params as well and not separate so we get:

message PostInitOptions {
    uint32 bits_per_label = 1;
    uint64 labels_per_unit = 2;
    uint32 min_num_units = 3;
    uint32 max_num_units = 4;
    uint32 compute_provider_id = 5; // A PostProvider id
    bool throttle = 6; // Throttle down setup phase computations while user is interactive on system
}

message ConfigResponse {
    PostInitOptions post_init_options = 1;
}

message StartSmeshingRequest {
    AccountId coinbase = 1;
    PostInitOptions post_init_options = 2;
}

I would make this commit breaking and get rid of all the responses that have only status in them, e.g. StopSmeshingResponse - the sooner we do this the better and it is a non-backward compatible change anyhow.

moshababo · 2021-06-27T08:58:30Z

there's a bit of duplication that can go away between PostInitOpts and ConfigResponse data items. Consider having a shareג object for post config params that is returned in ConfigResponse and is also passed by caller of CreatePostDataRequest.

The naming you specified are outdated, so plz use the latest. After some revisions, we now have 2 distinct configurations:

PoSTConfig (currently defined in PoSTConfigResponse) — protocol params only, defined via the node configuration. That’s why it's read-only.
PoSTSetupOpts — params for the local PoST setup (triggered only via StartSmeshing.

Considering this, your suggestion doesn’t make sense to me, so let me know if I missed something.

I would make this commit breaking and get rid of all the responses that have only status in them, e.g. StopSmeshingResponse - the sooner we do this the better and it is a non-backward compatible change anyhow.

I'd like to do it in a follow-up PR, because it doesn't involve solely the Smesher API.

avive · 2021-06-27T12:41:00Z

Considering this, you suggestion doesn’t make sense to me, so let me know if I missed something.

Ok, this makes sense for both items.

lrettig · 2021-06-29T20:56:23Z

proto/spacemesh/v1/smesher_types.proto

+        COMPUTE_API_CLASS_UNSPECIFIED = 0;
+        COMPUTE_API_CLASS_CPU = 1; // useful for testing on systems without a cuda or vulkan GPU
+        COMPUTE_API_CLASS_CUDA = 2;
+        COMPUTE_API_CLASS_VULKAN = 3;


Should we maybe add a COMPUTE_API_CLASS_OTHER?

For what reason? These value correspond to the type of providers supported by the gpu-post lib. If/when we'll add new supported types of providers to gpu-post then we'll add them to the enum in future api releases. Don't see how other is useful here.

proto/spacemesh/v1/smesher_types.proto

lrettig

I'm definitely not a fan of the PoST capitalization, e.g., PoSTConfig. I think PostConfig is much easier and less error prone.

avive · 2021-07-06T09:01:49Z

I'm definitely not a fan of the PoST capitalization, e.g., PoSTConfig. I think PostConfig is much easier and less error prone.

Same with me - we don't use this naming pattern anywhere in the Api and I agree that Post and PostConfig are better.

# Conflicts: # release/go/spacemesh/v1/types.pb.go

moshababo · 2021-07-26T04:51:28Z

I'm definitely not a fan of the PoST capitalization, e.g., PoSTConfig. I think PostConfig is much easier and less error prone.

Same with me - we don't use this naming pattern anywhere in the Api and I agree that Post and PostConfig are better.

Fixed (7e631f5, and in the implementation as well).

avive

LGTM

avive

As requested, dealing with empty GRPC responses will be done in another PR.

moshababo added 5 commits December 1, 2020 04:26

Update smesher API

d391501

Merge branch 'master' into new_smesher_api

8450d9c

# Conflicts: # proto/spacemesh/v1/smesher.proto # release/go/spacemesh/v1/smesher.pb.go # release/go/spacemesh/v1/smesher_types.pb.go

fix lint + build

560a13e

WIP

2ef8fb2

Some adjustments

2dbd0d3

moshababo requested review from avive and lrettig May 24, 2021 04:05

avive suggested changes May 24, 2021

View reviewed changes

proto/spacemesh/v1/smesher_types.proto Outdated Show resolved Hide resolved

proto/spacemesh/v1/smesher_types.proto Outdated Show resolved Hide resolved

proto/spacemesh/v1/smesher_types.proto Outdated Show resolved Hide resolved

More adjustments

81ae866

moshababo mentioned this pull request Jun 5, 2021

Smesher api implementation spacemeshos/smrepl#23

Merged

lrettig reviewed Jun 12, 2021

View reviewed changes

moshababo added 2 commits June 22, 2021 21:42

Update naming

1e18c71

debug CI

4d79a9b

moshababo force-pushed the new_smesher_api branch from aebcdb8 to 4d79a9b Compare June 27, 2021 07:45

Update documentation

5297d99

lrettig reviewed Jun 29, 2021

View reviewed changes

Update naming

7e631f5

moshababo force-pushed the new_smesher_api branch 2 times, most recently from 0631982 to 7e631f5 Compare July 18, 2021 07:59

moshababo added 2 commits July 18, 2021 11:11

Merge branch 'master' into new_smesher_api

eb70040

# Conflicts: # release/go/spacemesh/v1/types.pb.go

Merge branch 'master' of https://github.com/spacemeshos/api

f54df33

# Conflicts: # release/go/spacemesh/v1/types.pb.go

moshababo force-pushed the new_smesher_api branch from 91afd69 to f54df33 Compare July 18, 2021 08:11

fix: fields order

3679ef8

avive approved these changes Jul 26, 2021

View reviewed changes

avive self-requested a review July 26, 2021 05:10

avive approved these changes Jul 26, 2021

View reviewed changes

moshababo merged commit 9474f58 into master Jul 26, 2021

moshababo deleted the new_smesher_api branch July 26, 2021 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Smesher API #145

New Smesher API #145

moshababo commented May 24, 2021 •

edited

Loading

moshababo commented Jun 5, 2021

avive commented Jun 5, 2021

avive commented Jun 5, 2021

avive commented Jun 5, 2021 •

edited

Loading

avive commented Jun 5, 2021 •

edited

Loading

moshababo commented Jun 5, 2021

lrettig Jun 12, 2021

moshababo Jun 22, 2021

lrettig Jun 12, 2021

moshababo Jun 22, 2021

avive Jun 23, 2021

lrettig Jun 12, 2021

moshababo Jun 22, 2021

lrettig commented Jun 12, 2021

avive commented Jun 13, 2021

lrettig commented Jun 18, 2021

avive commented Jun 27, 2021 •

edited

Loading

moshababo commented Jun 27, 2021 •

edited

Loading

avive commented Jun 27, 2021 •

edited

Loading

lrettig Jun 29, 2021

avive Jun 29, 2021

lrettig left a comment

avive commented Jul 6, 2021

moshababo commented Jul 26, 2021

avive left a comment

avive left a comment

New Smesher API #145

New Smesher API #145

Conversation

moshababo commented May 24, 2021 • edited Loading

moshababo commented Jun 5, 2021

avive commented Jun 5, 2021

avive commented Jun 5, 2021

avive commented Jun 5, 2021 • edited Loading

avive commented Jun 5, 2021 • edited Loading

moshababo commented Jun 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrettig commented Jun 12, 2021

avive commented Jun 13, 2021

lrettig commented Jun 18, 2021

avive commented Jun 27, 2021 • edited Loading

moshababo commented Jun 27, 2021 • edited Loading

avive commented Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrettig left a comment

Choose a reason for hiding this comment

avive commented Jul 6, 2021

moshababo commented Jul 26, 2021

avive left a comment

Choose a reason for hiding this comment

avive left a comment

Choose a reason for hiding this comment

moshababo commented May 24, 2021 •

edited

Loading

avive commented Jun 5, 2021 •

edited

Loading

avive commented Jun 5, 2021 •

edited

Loading

avive commented Jun 27, 2021 •

edited

Loading

moshababo commented Jun 27, 2021 •

edited

Loading

avive commented Jun 27, 2021 •

edited

Loading