Replace snapshot with summarize to get summary of the container runtime

[sonalivdeshpande]

The deprecated snapshot function in packages\runtime\container-runtime\src\containerRuntime.ts was previously used to get a summary of the container runtime. This functionality has been replaced with the summarize funciton.

In order to remove it, the IRuntime interface must also be adjusted to not require an implementation of the snapshot function in file common\lib\container-definitions\src\runtime.ts.

Phases

• Adjust IRuntime interface to not require snapshot
• Remove snapshot from container-runtime

[scottn12]

In order to remove the snapshot function from the ContainerRuntime class, we must first not require it in the IRuntime interface. Since IRuntime is implemented in another class (ProxyRuntime), I propose we make snapshot an optional function in the following way:

export interface IRuntime extends IDisposable {
    /**
     * Snapshots the runtime
     */
    snapshot(tagMessage: string, fullTree?: boolean): Promise<ITree | null>;
}

to

export interface IRuntime extends IDisposable {
    /**
     * Snapshots the runtime
     */
    snapshot?(tagMessage: string, fullTree?: boolean): Promise<ITree | null>;
}

Does this seem like a good approach @ChumpChief @heliocliu @sdeshpande3 @ssimic2 ?

[heliocliu]

ProxyRuntime is from one of the example packages that shouldn't be being consumed in the same way so I don't think that needs special consideration here. So this should follow the standard procedure for back-compat (which may involve using the optional or not depending on usage of the snapshot function?)

[scottn12]

It seems that between the two classes which implement IRuntime, the snapshot function is not used anywhere in the FF repo or in partner repos, therefore we should be safe to simply remove the function from the interface and both classes.

[scottn12]

While testing removing snapshot from IRuntime I did find one usage which may need to be adjusted if we are to completely remove this function from the interface.

FluidFramework/packages/loader/container-loader/src/containerContext.ts

Line 215 in 0699d26

return this.runtime.snapshot(tagMessage, fullTree);

I would suggest to replace this instance of snapshot with summarize, however this.runtime is actually an instance of IRuntime, which does not declare summarize (summarize is declared in the ContainerRuntime class). Perhaps we should declare summarize in IRuntime?

[heliocliu]

Removing snapshot on IRuntime only to add summarize seems like a losing proposition. Do the reasons/context for deprecation/removal of snapshot on IRuntime also apply to ContainerContext (or IContainerContext depending on where it's declared there)? If so the solution is probably also to remove snapshot there as well, otherwise probably need to re-examine if it can be removed from IRuntime

[scottn12]

Actually, only snapshot on ContainerRuntime was deprecated. That is why I originally suggested we make snapshot optional on IRuntime. Maybe we should revisit that idea since it looks like snapshot is more involved than we initially thought.

#8286 (comment)

[scottn12]

@agarwal-navin, I noticed in your PR #4230, you mentioned that snapshot() would be removed in a follow up PR. Would you be able to provide any additional context or how you planned to fully remove this API?

[agarwal-navin]

Yeah, it has been a long goal to remove all the snapshot APIs. I removed it from most of the layers except from container, container runtime and container context.

We have couple of items tracking additional work for this:

• Update snapshots tests under packages/test/snapshots to use summarize instead of snapshot #6237.
• remove snapshot() path from Container #3929

The replay tool under packages/tools/replay-tool calls snapshot on the Container. Replacing it with summarize is going to take some work because:

1. The replay tool has to be updated to work with ISummaryTree instead of an ISnapshotTree.
2. summarize doesn't exist on Container and we don't want to add it there. We want to use either summarize on container runtime or summarizeOnDemand API on Summarizer.

[ssimic2]

The question here again is about loader/runtime backward compatibility. If we remove this API call from the runtime, older loader (container context) won't be able to get snapshot from new runtimes. We have few other tickets where we realized N-1 compatibility between loader and runtime is not sufficient. Once we do get into major release cadence this will probably be 2-3 year compatibility req. We need to figure out the compat. guidelines in the meantime. I assumed N-1 was correct based on this doc (needs a conversation, and then update): https://github.com/microsoft/FluidFramework/wiki/Compatibility-and-Versioning#nn-1-backwards-compatibility

[ssimic2]

These efforts don't seem blocked on loader/runtime compat. issue:

1. Work around removing snapshot on Container and container context (loader)
2. Reworking snapshot tests. I'm guessing this will be additive work, keeping existing snapshot validation.
3. Replay-tool work (related to 2.)

What is blocked is us removing runtime API, which is gated by above work + some wait time to flush the usage. Currently the minimum loader version that's in use seems to be 0.46 and we are regressing against 0.45+

[scottn12]

Closing this issue. After further discussion with @markfields, we decided to keep genericError and we will not be adding in a new error type.

[agarwal-navin]

Closing this issue. After further discussion with @markfields, we decided to keep genericError and we will not be adding in a new error type.

@scottn12 Did you close the wrong issue? I don't see anything related to error type here.

[ssimic2]

Hey @scottn12, it looks like you closed the wrong issue.