Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Container Runtime registry information is not currently extensible #4193

Closed
leeviana opened this issue Oct 30, 2020 · 15 comments
Closed

Container Runtime registry information is not currently extensible #4193

leeviana opened this issue Oct 30, 2020 · 15 comments
Assignees
Labels
design-required This issue requires design thought
Milestone

Comments

@leeviana
Copy link
Contributor

leeviana commented Oct 30, 2020

There are many scenarios in which there may be more Fluid DataStore metadata fields usable in the container runtime than just the two defined in FluidDataStoreRegistryEntry (which currently has subregistry and component factory support). However, it is currently not possible to actually extend the container factory registry info with more optional data because ContainerRuntime.load() expects the registry to be provided in the shape of Map<string, LazyPromise<FluidDataStoreRegistryEntry>> instead of Map<string, IFluidObjectThatProvidesFluidDataStoreRegistryEntryPromise>. Since we don't want to force a load of every lazy promise in the map to be able to access the data, we cannot actually expose our information off the container runtime's registry given this data shape.

We currently work around this by injecting top level container registry data into our handlers directly instead of using the container runtime's registry but this really breaks down when trying to use more "advanced" registry features like subregistries etc. (and we have no desire to reimplement all of the container registry features already natively implemented in the Fluid Framework). This also means that is it difficult to expose our extra data object registry information off of the standard fluidExport object in a way other containers can reasonably consume because expectations for that object are that it provides a FluidDataStoreRegistryEntry, not an IProvideFluidDataStoreRegistryEntry shape.

Here is an example "advanced" registry entry we export off of one of our data objects today:

// Consider exporting this on fluidExport.
export const registryInfo: ComponentRegistryData = {
  discoverData: getDiscoverData,
  factory: new LazyPromise(() =>
    import(/* webpackChunkName: "tableroComponent" */ './index').then((m) => m.TableroView.getFactory())
  ),
  preloadLocalizationAssets: (locale) => {
    // tslint:disable-next-line: no-floating-promises
    stringTableFetcher(locale);
  },
  manifest: [{ dataType: ComponentDataType.Table }],
  dataProvidersToPreload: [ComponentDataType.RichTextTableCell]
};

As you see, it contains the same FluidDataStoreRegistryEntry information but additionally contains preload pointers and discovery mechanisms that will by design want to exist before the factory promise is called.

I understand that this would be a breaking contract change with every existing container that uses ContainerRuntime.load... unless we did something weird like added a new optional param with the new shape etc. However, at the moment, trying to work around this dichotomy of registries is leading to unfortunately patterns and architecture choices (like having to recommend to partners to not use subregistries or collection patterns yet where they may actually be the optimal architectural choice for their data object).

@ghost ghost added the triage label Oct 30, 2020
@anthony-murphy anthony-murphy added the design-required This issue requires design thought label Oct 30, 2020
@skylerjokiel skylerjokiel self-assigned this Nov 3, 2020
@curtisman
Copy link
Member

curtisman commented Nov 4, 2020

So to put it succinctly, the problem is that the entry is hidden under the Promise, but you want to be able to look up (from the name) and get the information without loading the code?

I would want to understand more about the need to access using the name instead directly binding to it. The main purpose of the registry is for the runtime to able to "rehydrate" an instance from the snapshot. While we had allow symbolic creation using this pattern, I believe we are moving away from that and prefer direct call to create. Also I think we probably will restrict access to the registry as well. @vladsud to confirm.

@vladsud
Copy link
Contributor

vladsud commented Nov 4, 2020

I'd preface with - I do not fully understand the scenario (or maybe - did not put enough time to understand it :))
But here are some thoughts based on what I read in this thread:

  1. Overall, I really do not like a pattern of putting promises around objects that have 1-2 async only methods. It does not help in any way with anything to have Promise that returns Promise. In my view all such promises should be gone, and we should provide useful adapters to cover any interesting scenarios. This is separate, but I'd rather not see [string. factory] pair, I'd love to see just factory, and factory.type being unique string (something like "@ms/scriptor-<guid>" - i.e. unique, but also human readable)
  2. Same argument can be applied in reverse to existing system - i.e. having an extra promise does not make certain scenarios not possible (unless you try to sniff type of object). So I'm not sure how it helps with anything.
  3. And to Curtis point - yes, all these registries should be used only by runtime when loading data stores in file. Ideally all the rest should fall into two categories:
    • creation: through higher level abstractions, like DataObjectFactory.create*Instance() APIs.
  • loading: through handle resolution. All the existing code that tries to control that process (to add objects to certain eco-systems, like consumer/provider eco-system) should be converted to objects themselves doing it, i.e. some base class derived from DataObject that implements automatic registration of object in eco-system.

BTW, conversion in #1 can be done really easily:

  1. Add support for both promise and non-promise types on same argument. Mark old way deprecated
  2. wait for all consumers to move to non-promise
  3. remove support for promise thing

@leeviana
Copy link
Contributor Author

leeviana commented Nov 4, 2020

@curtisman Yes. We do directly bind to it today, however this direct binding system does not work with advanced registry features like subregistries which have different entrypoints into the system and are handled in some default way that we do not want to reimplement with regards to getting the keys and walking the trees etc. So effectively I cannot recommend that partners use things like subregistries even though they should because suddenly they lose perf features, the ability to use collection components etc. which all rely on static manifest information exposed by components.

@vladsud This goes way beyond simple loading/creating, we do things like prefetch/preload of localization, of data types (i.e. tablero code can say it has a data type dependency on a 'rich text' component type which will cause the container to prefetch that code too without binding to a specific component implementation) and enabling usage of collection components without custom logic tied to those component's implementations. It is not sufficient for us to just load/create components, we need to be smart about how we do it without adding in direct references/dependencies between components/containers.

Ultimately we would like our registry and component registry entries to be at the very least compatible with the container component registry concept, otherwise things get very messy. Hence this ask. The other alternative is to build a completely new set of registry conventions and wrappers, reimplement subregistry usage and any other Fluid Framework registry patterns, force all containers to use specific root component implementations for APIs and create our own version of "advancedFluidExport" that component developers will have to use instead of "fluidExport". I do not want this level of diversion or this amount of work reimplementing a concept that is simply parallel to the existing concept/code in the Framework.

@vladsud
Copy link
Contributor

vladsud commented Nov 4, 2020

ContainerRuntime internally works with IFluidDataStoreRegistry interface, and I think that's correct. I'd push NamedFluidDataStoreRegistryEntries type out of couple layers in favor of IFluidDataStoreRegistry.
And would go even further - I'd say instead of using things like NamedFluidDataStoreRegistryEntries for composition of data that eventually gets converted to IFluidDataStoreRegistry , I'd rather operate only with one interface - IFluidDataStoreRegistry, and provide useful adapters to merge two instances of IFluidDataStoreRegistry, to create from list of tries (current FluidDataStoreRegistry, but we can easily add more wrappers that take input in different formats, including not having promises), etc.

Basically runtime layers should only work with one concept - IFluidDataStoreRegistry, everything else should be pushed out from "kernel" mode into "user" mode :)

@leeviana
Copy link
Contributor Author

leeviana commented Nov 9, 2020

As long as the resulting solution is extensible enough to allow the container/component interactions to set up their registry metadata (via fluidExport conventions) in a way that lets us use subcomponents registry data, I don't care what the runtime internals/contracts specifically require/depend on. Also it might be interesting to note that today the subcomponent registry pattern actually takes in yet another different definition of a registry entry (promise instead of lazy promise?), which probably also needs to be reconciled with this as all components should want to expose the same factory logic regardless of whether or not they are used in the container layer or subcomponent layer or both.

Today, I am recommending to all teams creating components to not use the collection pattern, even though that might be the correct choice for their scenario, because then their components cannot be consumed in subregistries (we cannot get access to subregistry manifest information without essentially reimplementing all the framework registry logic). The alternative is making everything a top level container global and essentially ignoring the subregistry feature but that seems less than ideal in terms of designing component dependencies that don't rely on container knowledge of all that component's dependencies.

From a component developer perspective they should be able to design their entire component based on their component's needs and dependencies without touching consuming container code. This means deciding on collection component vs not and their subregistry dependencies without having to worry about losing performance optimizations/lazy loading and whether or not collection singleton management at the top container level will be able to support them.

@vladsud
Copy link
Contributor

vladsud commented Nov 9, 2020

I believe fluidExport is already in good shape - code around it assumes one of these types being exposed:
IProvideRuntimeFactory
IProvideFluidDataStoreFactory
IProvideFluidDataStoreRegistry
IProvideFluidCodeDetailsComparer

These are interfaces, they do not assume any specific data representation underneath, and result of calling these interfaces is either object objects, or interfaces from this list. I.e. we do not have another way to represent that data at runtime level.
These interfaces are the same interfaces container runtime expects (middle two). So I think we are very consistent here.

I think the place that is not consistent is how Aqueduct represents registry - I mentioned it in PR #4246 as a next step of improvement. It should either take IProvideFluidDataStoreRegistry (and push the problem out of how initial data is represented), or take it in wider variety of forms (that's actually supported by that same PR - we can convert to IProvideFluidDataStoreRegistry now 3 more forms, I think adding IProvideFluidDataStoreRegistry itself as an accepted type would completely make it "do what you want".

I think collections question is orthogonal to all of that, as it's eternal dependency to a data object, not its child data strore. How it's organized should not matter as long as there is a way to reach to it (collection) and its entities (elements) through handles / IFluidLodable / requests (which are all synonyms).

@leeviana
Copy link
Contributor Author

Note: The request is for fluidExport to be typed as IFluidObject (can expose all of these IProvides still) so that components could expose even more APIs/behaviors off of their fluidExports (like the information mentioned in the original issue post)

@ghost ghost added the status: stale label Dec 14, 2021
@ghost
Copy link

ghost commented Dec 14, 2021

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

@leeviana
Copy link
Contributor Author

Same state still

@ghost ghost removed the status: stale label Dec 14, 2021
@ghost ghost added the status: stale label Jun 13, 2022
@ghost
Copy link

ghost commented Jun 13, 2022

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

@leeviana
Copy link
Contributor Author

I believe fluid export is still strongly typed instead of provider typed so that cannot be reused, however we are currently investigating working around this by adding provider contracts to the data object factory.

@ghost ghost removed the status: stale label Jun 13, 2022
@ghost ghost added the status: stale label Dec 11, 2022
@ghost
Copy link

ghost commented Dec 11, 2022

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

@leeviana
Copy link
Contributor Author

We have worked around this by adding our own data object factory layer, however this current issue is still true in terms of framework implementation. Maybe this will all be a moot point when we decide to put our data object factory implementation back into shared client code.

@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

@leeviana
Copy link
Contributor Author

We have put this in our own data object factory layer for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-required This issue requires design thought
Projects
None yet
Development

No branches or pull requests

5 participants