Feature/biuld simulators suggestions #470
Conversation
| this.simulator.setTrajectoryDataHandler( | ||
| this.visData.parseAgentsFromNetData.bind(this.visData) | ||
| ); |
There was a problem hiding this comment.
I'm into using this for all of them!
| public get octopusClient(): OctopusServicesClient { | ||
| if (!this._octopusClient || !this._octopusClient.socketIsValid()) { | ||
| throw new Error( | ||
| "Remote Octopus client is not configured or socket is invalid." | ||
| ); | ||
| } | ||
| return this.octopusClient; | ||
| return this._octopusClient; |
| export const getClassFromParams = (params: SimulatorParams) => { | ||
| if ("netConnectionSettings" in params) { | ||
| return { | ||
| simulatorClass: RemoteSimulator, | ||
| typedParams: params as RemoteSimulatorParams, | ||
| }; | ||
| } else if ("clientSimulatorImpl" in params) { | ||
| return { | ||
| simulatorClass: ClientSimulator, | ||
| typedParams: params as ClientSimulatorParams, | ||
| }; | ||
| } else if ("simulariumFile" in params) { | ||
| return { | ||
| simulatorClass: LocalFileSimulator, | ||
| typedParams: params as LocalFileSimulatorParams, | ||
| }; | ||
| } else { | ||
| return { | ||
| simulatorClass: null, | ||
| typedParams: null, | ||
| }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I like this! It's the benefits of the old type field I had but without cluttering up the params.
interim17
left a comment
interim17
left a comment
There was a problem hiding this comment.
Left a few comments. Mostly stoked on the organization and the way you're handling the params, but not sure we are on the same page re: WebsocketClient. If you feel strongly about it probably best to pair or talk about it?
This branch currently can't load any simulations, as it gives network config errors, I haven't started trying to piece them apart yet.
|
|
||
| getWebsocket(): WebsocketClient | null; |
There was a problem hiding this comment.
In general I like all these changes, this is the only part I don't really understand the motivation.
We refactored ISimulator to get WebSocket related code out of it, and reduce the number of unimplemented functions and methods floating around, like getWebsocket on a client or local file sim. The octopus client, remote metrics calculator and remote simulator all use it and keeping at the controller level makes it easy to make sure they all are on the same session.
This branch does truthy checks for this.remoteWebsocketClient on the controller but it seems like you removed that variable from the controller class, so it's always going to make a new one, and then the octopus client and metrics calculator will be on a different connection from the simulator.
What's the motivation to push the websocket client down from controller into the simulator?
There was a problem hiding this comment.
oh for me, it just makes sense to only make a remote client if you're a remote simulator. And it should be the job of the class to handle the kind of connection that simulator needs. I made a getter for the client that gets it from the simulator, so it won't remake it if the remote client has been initiated. I'd like to talk to you and Dan about this, I wasn't aware that it used to be in there.
The other reason for doing it was just so you can send the params straight into the simulator class without having to check to see if it's a remote simulator and then making a websocket first. Again, it seems like a class should know how to initialize itself
| } | ||
| public configureOctopus(config: NetConnectionParams): Promise<string> { | ||
| const webSocketClient = | ||
| this.remoteWebsocketClient || |
There was a problem hiding this comment.
You don't initialize this.remoteWebsocketClient on the controller in this branch
There was a problem hiding this comment.
I'm not saying this function couldn't be improved, but the key piece is assigning this.remoteWebsocketClient and then using that same instance when we configure the remote simulator:
public configureNetwork(config: NetConnectionParams): Promise<string> {
if (
!this.remoteWebsocketClient ||
!this.remoteWebsocketClient.socketIsValid()
) {
this.octopusClient = undefined;
this.remoteWebsocketClient = new WebsocketClient(
config,
this.onError
);
}
if (!this.octopusClient) {
this.octopusClient = new OctopusServicesClient(
this.remoteWebsocketClient
);
}
return this.octopusClient.connectToRemoteServer();
}
........
const simulator = new RemoteSimulator(
this.remoteWebsocketClient,
this.onError,
params.requestJson
);
| public configureNetwork(config: NetConnectionParams) { | ||
| if (!this.needsNewNetworkConfig(config)) { | ||
| return this.webSocketClient; | ||
| } | ||
| if (!this.webSocketClient || !this.webSocketClient.socketIsValid()) { | ||
| this.webSocketClient = new WebsocketClient( | ||
| config, | ||
| this.handleError | ||
| ); | ||
| } | ||
| return this.webSocketClient; | ||
| } |
There was a problem hiding this comment.
If we make a new WebsocketClient here I'm not sure we'll be able to receive converted files or the smoldyn data results anymore.
There was a problem hiding this comment.
it gets passed back up in the getter, so it should be accessible
| import { ISimulariumFile } from "../ISimulariumFile"; | ||
| import { IClientSimulatorImpl } from "../localSimulators/IClientSimulatorImpl"; | ||
| import { NetConnectionParams } from "../WebsocketClient"; | ||
|
|
||
| export interface BaseSimulatorParams { | ||
| fileName: string; | ||
| } | ||
|
|
||
| export interface RemoteSimulatorParams extends BaseSimulatorParams { | ||
| netConnectionSettings?: NetConnectionParams; | ||
| requestJson?: boolean; | ||
| prefetchFrames?: boolean; | ||
| } | ||
|
|
||
| export interface ClientSimulatorParams extends BaseSimulatorParams { | ||
| clientSimulatorImpl?: IClientSimulatorImpl; | ||
| } | ||
|
|
||
| export interface LocalFileSimulatorParams extends BaseSimulatorParams { | ||
| simulariumFile?: ISimulariumFile; | ||
| geoAssets?: { [key: string]: string }; | ||
| } | ||
|
|
||
| export type SimulatorParams = | ||
| | RemoteSimulatorParams | ||
| | ClientSimulatorParams | ||
| | LocalFileSimulatorParams; |
There was a problem hiding this comment.
I really like having this stuff in its own directory!
| private get remoteWebsocketClient(): WebsocketClient | null { | ||
| if (!this.simulator) { | ||
| return null; | ||
| } | ||
| return this.simulator?.getWebsocket(); |
There was a problem hiding this comment.
this redefined the remoteWebsocketClient as the one that is on the simulator. so now when you do this.remoteWebsocketClient it gets the one on the simulator.
I didn't do any debugging to make sure things happen in the correct order, so it probably needs an "initialize octopus client" call on creation of the simulator but there is no real technical difference as far as being able to access it between storing it directly on the controller class or on a class that in held on controller class
There was a problem hiding this comment.
What is the benefit of having the websocket client live on the simulator though? It requires that we add getWeboscket to all non-remote simulators, and if it lives on the controller it can easily be accessed by all three abstractions that use it (octopusClient, remoteMetricsCalculator, remoteSimulator). The remoteWebsocketClient has lived on the controller as long as I've been here.
If it lives on the simulator, when we switch to a new client simulator or local file, we lose the websocket connection, and have to reconnect if we want to talk to Octopus, whereas if it lives on the controller, we can switch simulators without losing our websocket connection.
Unless I'm missing something the main benefit is trading strongly typed params in favor of getWebsocket on ISimulator so you can do a really concise type guard with getClassFromParams and initialize with new simulatorClass but I think the strong typing on the params, and being able to initialize the simulators with different non-optional param fields is a benefit and definitely worth the extra conditional checks.
There was a problem hiding this comment.
I'm am coming at this from some distance, but to me, if you're only ever initializing something if you have a certain class, then that class should own it. IE you only need a remote client if you're a remote simulator. but if you're saying all these other things are using it, then you should always initialize the client and then have it ready when you need it. But right now we're conditionally making it based on user settings, but then also keeping it independent of user settings?
If you never load a networked file, do you still make an octopus client? Do you only make it when you do the file conversion ?
I'm still a little shaky on why we have the two clients too.
If you load a networked file and then go to a local file, does the web socket just stay open?
What I guess I'm getting at is this seems like something in-between a user setting (network params) and internals.
There was a problem hiding this comment.
The primary reason to create the second client was to separate concerns and be more strict about what the ISimulator interface is for. I'm not sure if/how much you want me to elaborate on that.
We could choose to store fallback net connection settings and always open a web socket on load, but to date we have waited for the user to provide the settings with a call of changeFile or controller params.
So for example when you load your 2D sim in binding simulator we don't need one, so we don't make one. And if we fetch directly from someone's dropbox we don't make one, which I think is how you get your 3D sim in the binding sim as well right? It's not on octopus?
Right now you make a web socket connection if a user:
- requests a networked trajectory or simulator
- makes an auto-conversion request
- asks to query the bio-simulators API
Then the connection is created and passed to the classes that use it, the benefit being that some of those classes are ephemeral and the connection can outlive them.
Currently we always create new simulators on a per-trajectory basis, even when we switch between remote trajectories, so if the simulator owns the web socket connection, then every time you switch simulations you have to tear it down and reconnect. The only case where we kept the simulator around before was auto-conversion, before the octopusClient handled it, and that added some complexity for front ends to handle. I've noticed that switching is faster on the branch I've been working on if I go client -> remote -> local -> remote, as we keep the same websocket connection.
You're right that we could hold off on making an octopusClient in cases where we only need a simulator, we would just make configureNetwork and configureOctopusClient distinct.
Suggestions shown here: