feat: Add an optional argument to set memory allocation by dsarlis · Pull Request #596 · dfinity/sdk

dsarlis · 2020-04-27T15:22:14Z

This patch adds an optional parameter to dfx canister install which allows the user to set a memory allocation for their canister.

I've modeled this similarly to the param for compute allocation. Let me know what you think and also feel free to push commits on this branch if it's easier and faster.

I'm also not sure what kind of testing you'd expect for this. I've used this patch to present in the last global R&D, so it works (at least it did the job I wanted).

nomeata · 2020-04-27T15:27:10Z

src/dfx/src/commands/canister/install.rs

+                .long("memory-allocation")
+                .short("m")
+                .takes_value(true)
+                .default_value("8GB")


Do you want to duplicate the information about the default here? I think the two sensible design spaces are:

The system defines a default, dfx will, if no --memory-allocation is passed, send none, system default applies

The system defines no default, the field is mandatory, dfx determines (here) what the default is.

With this code, there are now two components both defining a default value; that’s odd.

I'm fine with the first approach.

Btw, the same happens for compute allocation and I don't remember why it was done like that.

With this code, there are now two components both defining a default value; that’s odd.

No, there's two components that follow the default value set by a common reference (the public spec) :) But yeah it's probably redundant since the replica will enforce the default anyway.

I wouldn’t consider it “following the public spec” if the client makes an optional field mandatory and hides the default value (which may change in later versions). But this is getting philosophical and we are in agreement :-)

Maybe the public spec shouldn’t be the place to define default values… oh well.

same happens for compute allocation

Yes, we should do the same for that too.

Maybe the public spec shouldn’t be the place to define default values… oh well.

This would mean that a concrete Internet Computer implementation could choose an arbitrary (within the specified range) default value if none is specified, which would mean that if I use dfx against two different implementations of the IC, then I get different behavior for the same request. Maybe it's cool, maybe it's not.

Anyway, I already fell in your trap

But this is getting philosophical and we are in agreement :-)

and as I said I'm fine with your first suggestion.

I'd much prefer the first approach.

No, there's two components that follow the default value set by a common reference (the public spec) :)

I think the mistake is treating it as a "default value" at the entry point. The spec just says if the value isn't specified the system should treat it like 2^33. That's at the instanciation point. Technically we could not have a field at all and still be spec compliant.

The interface definition is clear that the field is optional, so there should be nothing on the wire.

Glad to see everyone in agreement :-)

nmattia · 2020-04-27T15:48:58Z

nix/sources.json

    },
    "dfinity": {
-        "ref": "v0.5.4",
+        "ref": "master",


is this only for testing?

Well, yes and no :)

I needed to point to a commit past v0.5.5 in dfinity to make things work for my demo last week. I'll use what I'm told here (presumably v0.5.6). Better yet, if someone bumps dfinity separately (say for a new SDK release) until this is merged and it includes the commit I'm interested in, I don't need to touch this at all.

Great, muting the thread without approving. Just ping us if you do need a change that needs an infra review!

src/dfx/src/commands/canister/install.rs

src/ic_http_agent/src/types/canister_attributes.rs

dsarlis

I've made the requested changes. Specifically, the main change is that memory allocation was turned into a truly optional field for dfx, so if it's not set it's not even serialized into the CBOR request sent over to the replica.

src/dfx/src/lib/message.rs

lsgunnlsgunn

Usage message looks reasonable.

hansl · 2020-04-28T18:50:11Z

This LGTM, but is blocked on NOT moving sources.json to master, which blocks the PR. As soon as Replica has a release with this change in, we can patch this PR and make it in.

dsarlis · 2020-04-29T07:54:48Z

This LGTM, but is blocked on NOT moving sources.json to master, which blocks the PR. As soon as Replica has a release with this change in, we can patch this PR and make it in.

SGTM. Is there an expected date to get v0.5.7? I checked v0.5.6 and it still doesn't include the commit I need.

dsarlis · 2020-05-12T10:06:43Z

This LGTM, but is blocked on NOT moving sources.json to master, which blocks the PR. As soon as Replica has a release with this change in, we can patch this PR and make it in.

@hansl Looks like there's been a v0.5.7 release that includes the changes needed on the replica side, so I was able to drop the changes to sources.json. PTAL and let me know if we need anything else here.

nomeata · 2020-05-12T10:23:32Z

I should point out that if we want to be strict, the replica shoundn’t expose this feature until it has been part of a spec release (it’s not in 0.2), and therefore the SDK shouldn't be in it before.

See https://github.com/dfinity-lab/ic-ref/issues/44 for the ongoing disucssion what goes into 0.4.

Given the fuzz arond “focus on Tungston” it is not clear if memory allocation will make it. (Maybe the argument “it’s already almost there” will apply.)

Whether we want to be strict in using the spec to plan an implement features is not my call to make. It unquestionably adds delays and some friction.

dsarlis · 2020-05-12T10:36:16Z

I should point out that if we want to be strict, the replica shoundn’t expose this feature until it has been part of a spec release (it’s not in 0.2), and therefore the SDK shouldn't be in it before.

The replica already supports this feature (for like a month now). Are you suggesting that I revert the change?

See dfinity-lab/ic-ref#44 for the ongoing disucssion what goes into 0.4.

Given the fuzz arond “focus on Tungston” it is not clear if memory allocation will make it. (Maybe the argument “it’s already almost there” will apply.)

I assume you meant "focus on Tungsten". My understanding is that the memory allocation is something we want to be included for Tungsten and I am fairly certain that's @granstrom vision as well. Johan please correct me if I'm wrong.

Whether we want to be strict in using the spec to plan an implement features is not my call to make. It unquestionably adds delays and some friction.

The way I see it, if you can add a feature in a non-breaking way (memory allocation is such a case because there's a sensible default that is used even if SDK doesn't support it) then I don't see any problem looking ahead and implementing features that will be needed in general. But it's certainly not my call either how strict we want to be.

dsarlis · 2020-05-12T10:37:05Z

Also perhaps we should move the discussion about including memory allocation or not on the issue you mentioned to avoid unnecessary noise in the current PR.

nomeata · 2020-05-12T10:39:21Z

Also perhaps we should move the discussion about including memory allocation or not on the issue you mentioned to avoid unnecessary noise in the current PR.

Yes, absolute! Actually, the Google doc linked from that PR is the right place to discuss. I just wanted to increase visibility of that discussion

dsarlis · 2020-10-06T13:23:31Z

A flag to set the memory allocation seems to be already available in latest dfx (checked against 0.6.10). Closing the current PR.

Add an optional argument to set memory allocation

7d4ca3a

dsarlis requested a review from a team April 27, 2020 15:22

dsarlis requested a review from a team as a code owner April 27, 2020 15:22

nomeata reviewed Apr 27, 2020

View reviewed changes

nmattia reviewed Apr 27, 2020

View reviewed changes

hansl reviewed Apr 27, 2020

View reviewed changes

src/dfx/src/commands/canister/install.rs Outdated Show resolved Hide resolved

hansl reviewed Apr 27, 2020

View reviewed changes

src/ic_http_agent/src/types/canister_attributes.rs Outdated Show resolved Hide resolved

hansl reviewed Apr 27, 2020

View reviewed changes

src/ic_http_agent/src/types/canister_attributes.rs Show resolved Hide resolved

hansl suggested changes Apr 27, 2020

View reviewed changes

src/ic_http_agent/src/types/canister_attributes.rs Outdated Show resolved Hide resolved

dsarlis added 2 commits April 28, 2020 10:46

Merge branch 'master' into dimitris/memory-allocation

2cac422

Address review feedback

cac5ed2

dsarlis commented Apr 28, 2020

View reviewed changes

src/dfx/src/lib/message.rs Show resolved Hide resolved

Fix formatting

5e232c6

dsarlis requested a review from hansl April 28, 2020 10:06

dsarlis mentioned this pull request Apr 28, 2020

fix: Use None instead of 0 if no compute allocation is specified #601

Merged

Use map instead of match

8a85987

dsarlis changed the title ~~Add an optional argument to set memory allocation~~ feat: Add an optional argument to set memory allocation Apr 28, 2020

hansl requested a review from lsgunnlsgunn April 28, 2020 17:27

lsgunnlsgunn reviewed Apr 28, 2020

View reviewed changes

hansl approved these changes Apr 28, 2020

View reviewed changes

dsarlis added 2 commits May 7, 2020 13:32

Merge branch 'master' into dimitris/memory-allocation

7b4027a

Merge branch 'master' into dimitris/memory-allocation

f15caf1

dsarlis closed this Oct 6, 2020

lwshang deleted the dimitris/memory-allocation branch July 29, 2022 19:00

Comments

Conversation

dsarlis commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dsarlis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsgunnlsgunn left a comment

Choose a reason for hiding this comment

Uh oh!

hansl commented Apr 28, 2020

Uh oh!

dsarlis commented Apr 29, 2020

Uh oh!

dsarlis commented May 12, 2020

Uh oh!

nomeata commented May 12, 2020

Uh oh!

dsarlis commented May 12, 2020

Uh oh!

dsarlis commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nomeata commented May 12, 2020

Uh oh!

dsarlis commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dsarlis commented Apr 27, 2020 •

edited

Loading

dsarlis left a comment •

edited

Loading

dsarlis commented May 12, 2020 •

edited

Loading