Skip to content

Feature/biuld simulators suggestions #470

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.

Sign up for GitHub

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

Draft
meganrm wants to merge 3 commits into
base: feature/build-simulators
Choose a base branch
from
Draft

Feature/biuld simulators suggestions #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52424f1
move files and combine some code
meganrm Mar 18, 2025
19df28c
merge more of the code and push netconnection down
meganrm Mar 19, 2025
0dc272d
better error handling
meganrm Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 65 additions & 99 deletions src/controller/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,27 @@ import {
FILE_STATUS_SUCCESS,
FILE_STATUS_FAIL,
PlotConfig,
SimulatorParams,
} from "../simularium/types.js";

import { ClientSimulator } from "../simularium/ClientSimulator.js";
import { ISimulator } from "../simularium/ISimulator.js";
import { LocalFileSimulator } from "../simularium/LocalFileSimulator.js";
import {
getClassFromParams,
ISimulator,
} from "../simularium/Simulator/ISimulator.js";
import { LocalFileSimulator } from "../simularium/Simulator/LocalFileSimulator.js";
import { FrontEndError } from "../simularium/FrontEndError.js";
import type { ISimulariumFile } from "../simularium/ISimulariumFile.js";
import { WebsocketClient } from "../simularium/WebsocketClient.js";
import { TrajectoryType } from "../constants.js";
import { RemoteMetricsCalculator } from "../simularium/RemoteMetricsCalculator.js";
import { OctopusServicesClient } from "../simularium/OctopusClient.js";
import {
isLocalProceduralSimulatorParams,
isLocalFileSimulatorParams,
isRemoteSimulatorParams,
} from "../util.js";
import { isLocalFileSimulatorParams } from "../util.js";
import { SimulatorParams } from "../simularium/Simulator/types.js";

jsLogger.setHandler(jsLogger.createDefaultHandler());

export default class SimulariumController {
public simulator?: ISimulator;
public remoteWebsocketClient?: WebsocketClient;
public octopusClient?: OctopusServicesClient;
public _octopusClient?: OctopusServicesClient;
public metricsCalculator?: RemoteMetricsCalculator;
public visData: VisData;
public visGeometry: VisGeometry | undefined;
Expand Down Expand Up @@ -75,60 +72,45 @@ export default class SimulariumController {
this.cancelCurrentFile = this.cancelCurrentFile.bind(this);
}

public buildSimulator(params: SimulatorParams): ISimulator {
if (isLocalProceduralSimulatorParams(params)) {
const simulator = new ClientSimulator(params.clientSimulatorImpl);
simulator.setTrajectoryDataHandler(
this.visData.parseAgentsFromNetData.bind(this.visData)
);
return simulator;
} else if (isLocalFileSimulatorParams(params)) {
const simulator = new LocalFileSimulator(
params.fileName,
params.simulariumFile
);
if (
this.visGeometry &&
params.geoAssets &&
!isEmpty(params.geoAssets)
) {
this.visGeometry.geometryStore.cacheLocalAssets(
params.geoAssets
);
}
simulator.setTrajectoryDataHandler(
this.visData.parseAgentsFromFrameData.bind(this.visData)
);
return simulator;
} else if (isRemoteSimulatorParams(params)) {
if (this.needsNewNetworkConfig(params.netConnectionSettings)) {
this.configureNetwork(params.netConnectionSettings);
}
if (!this.remoteWebsocketClient) {
throw new Error("Websocket client not configured");
}
private handleError(message: string): void {
if (this.onError) {
return this.onError(new FrontEndError(message));
} else {
throw new Error(message);
}
}

const simulator = new RemoteSimulator(
this.remoteWebsocketClient,
this.onError,
params.requestJson
);
simulator.setTrajectoryDataHandler(
this.visData.parseAgentsFromNetData.bind(this.visData)
);
return simulator;
public initSimulator(params: SimulatorParams) {
if (!params) {
this.handleError("Invalid simulator configuration");
}
if (!params.fileName) {
this.handleError("Invalid simulator configuration: no file name");
}
const { simulatorClass, typedParams } = getClassFromParams(params);
if (!simulatorClass) {
this.handleError("Invalid simulator configuration");
return;
}
throw new Error("Invalid simulator configuration");
if (
this.visGeometry &&
"geoAssets" in params &&
!isEmpty(params.geoAssets)
) {
this.visGeometry.geometryStore.cacheLocalAssets(params.geoAssets);
}
// will throw an error if the params are invalid
return new simulatorClass(typedParams, this.onError);
}

private createSimulatorConnection(params: SimulatorParams): void {
try {
this.simulator = this.buildSimulator(params);
} catch (err) {
console.error("createSimulatorConnection failed", err);
throw err;
this.simulator = this.initSimulator(params);
if (!this.simulator) {
return;
}

this.simulator.setTrajectoryDataHandler(
this.visData.parseAgentsFromNetData.bind(this.visData)
);
Comment on lines +111 to +113
Copy link
Contributor

@interim17 interim17 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm into using this for all of them!

this.simulator.setTrajectoryFileInfoHandler(
(trajFileInfo: TrajectoryFileInfo) => {
this.handleTrajectoryInfo(trajFileInfo);
Expand All @@ -137,22 +119,12 @@ export default class SimulariumController {
this.playBackFile = params.fileName;
}

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
);
}
public configureOctopus(config: NetConnectionParams): Promise<string> {
const webSocketClient =
this.remoteWebsocketClient ||
Copy link
Contributor

@interim17 interim17 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't initialize this.remoteWebsocketClient on the controller in this branch

Copy link
Contributor

@interim17 interim17 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
            );

new WebsocketClient(config, this.onError);

this._octopusClient = new OctopusServicesClient(webSocketClient);

return this.octopusClient.connectToRemoteServer();
}
Expand All @@ -161,27 +133,17 @@ export default class SimulariumController {
return !!this.simulator;
}

public get ensureOctopusClient(): OctopusServicesClient {
if (!this.octopusClient || !this.octopusClient.socketIsValid()) {
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;
Comment on lines +136 to +142
Copy link
Contributor

@interim17 interim17 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

}

public isRemoteOctopusClientConfigured(): boolean {
return !!(this.octopusClient && this.octopusClient?.socketIsValid());
}

private needsNewNetworkConfig(
netConnectionConfig: NetConnectionParams
): boolean {
const expectedIp = `wss://${netConnectionConfig.serverIp}:${netConnectionConfig.serverPort}/`;
return (
!this.remoteWebsocketClient ||
this.remoteWebsocketClient.getIp() !== expectedIp
);
return !!(this.octopusClient && this.octopusClient.socketIsValid());
}

public get isChangingFile(): boolean {
Expand Down Expand Up @@ -322,7 +284,7 @@ export default class SimulariumController {
netConnectionSettings: netConnectionConfig,
fileName,
});
this.ensureOctopusClient.setOnConversionCompleteHandler(() => {
this.octopusClient.setOnConversionCompleteHandler(() => {
this.start();
});
}
Expand All @@ -340,7 +302,7 @@ export default class SimulariumController {

try {
this.setupConversion(netConnectionConfig, fileName);
return this.ensureOctopusClient.convertTrajectory(
return this.octopusClient.convertTrajectory(
dataToConvert,
fileType,
fileName
Expand All @@ -361,10 +323,7 @@ export default class SimulariumController {
): Promise<void> {
try {
this.setupConversion(netConnectionConfig, fileName);
return this.ensureOctopusClient.sendSmoldynData(
fileName,
smoldynInput
);
return this.octopusClient.sendSmoldynData(fileName, smoldynInput);
} catch (e) {
return Promise.reject(e);
}
Expand All @@ -383,15 +342,22 @@ export default class SimulariumController {
netConnectionConfig: NetConnectionParams
): void {
if (!this.isRemoteOctopusClientConfigured()) {
this.configureNetwork(netConnectionConfig);
this.configureOctopus(netConnectionConfig);
}

this.ensureOctopusClient.setHealthCheckHandler(handler);
this.ensureOctopusClient.checkServerHealth();
this.octopusClient.setHealthCheckHandler(handler);
this.octopusClient.checkServerHealth();
}

public cancelConversion(): void {
this.ensureOctopusClient.cancelConversion();
this.octopusClient.cancelConversion();
}

private get remoteWebsocketClient(): WebsocketClient | null {
if (!this.simulator) {
return null;
}
return this.simulator?.getWebsocket();
Comment on lines +356 to +360
Copy link
Contributor Author

@meganrm meganrm Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@interim17 interim17 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@meganrm meganrm Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@interim17 interim17 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private setupMetricsCalculator(
Expand Down
18 changes: 14 additions & 4 deletions src/simularium/ClientSimulator.ts → src/simularium/Simulator/ClientSimulator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import jsLogger from "js-logger";
import { ILogger } from "js-logger";

import { VisDataMessage, TrajectoryFileInfo } from "./types.js";
import { VisDataMessage, TrajectoryFileInfo } from "../types.js";
import {
ClientMessageEnum,
ClientPlayBackType,
IClientSimulatorImpl,
} from "./localSimulators/IClientSimulatorImpl.js";
} from "../localSimulators/IClientSimulatorImpl.js";
import { ISimulator } from "./ISimulator.js";
import { ClientSimulatorParams } from "./types.js";
import { WebsocketClient } from "../WebsocketClient.js";

// a ClientSimulator is a ISimulator that is expected to run purely in procedural javascript in the browser client,
// with the procedural implementation in a IClientSimulatorImpl
Expand All @@ -22,7 +24,11 @@ export class ClientSimulator implements ISimulator {
public onTrajectoryDataArrive: (msg: VisDataMessage) => void;
public handleError: (error: Error) => void;

public constructor(sim: IClientSimulatorImpl) {
public constructor(params: ClientSimulatorParams) {
const { clientSimulatorImpl } = params;
if (!clientSimulatorImpl) {
throw new Error("ClientSimulator requires a IClientSimulatorImpl");
}
this.logger = jsLogger.get("netconnection");
this.logger.setLevel(jsLogger.DEBUG);

Expand All @@ -35,7 +41,11 @@ export class ClientSimulator implements ISimulator {
this.handleError = () => {
/* do nothing */
};
this.localSimulator = sim;
this.localSimulator = clientSimulatorImpl;
}

public getWebsocket(): WebsocketClient | null {
return null;
}

public setTrajectoryFileInfoHandler(
Expand Down
38 changes: 37 additions & 1 deletion src/simularium/ISimulator.ts → src/simularium/Simulator/ISimulator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,45 @@
import { VisDataMessage, TrajectoryFileInfo } from "./types.js";
import { VisDataMessage, TrajectoryFileInfo } from "../types.js";
import { WebsocketClient } from "../WebsocketClient.js";
import { ClientSimulator } from "./ClientSimulator.js";
import { LocalFileSimulator } from "./LocalFileSimulator.js";
import { RemoteSimulator } from "./RemoteSimulator.js";
import {
ClientSimulatorParams,
LocalFileSimulatorParams,
RemoteSimulatorParams,
SimulatorParams,
} from "./types.js";

/**
From the caller's perspective, this interface is a contract for a
simulator that can be used to control set up, tear down, and streaming,
and to subscribe to data events and error handling.
*/

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,
};
}
};
Comment on lines +19 to +41
Copy link
Contributor

@interim17 interim17 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! It's the benefits of the old type field I had but without cluttering up the params.


export interface ISimulator {
/**
* Callbacks to subscribe front end implementations
Expand Down Expand Up @@ -46,4 +80,6 @@ export interface ISimulator {
requestFrameByTime(time: number): void;
/** request trajectory metadata */
requestTrajectoryFileInfo(fileName: string): void;

getWebsocket(): WebsocketClient | null;
Comment on lines +83 to +84
Copy link
Contributor

@interim17 interim17 Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@meganrm meganrm Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
23 changes: 20 additions & 3 deletions src/simularium/LocalFileSimulator.ts → ...imularium/Simulator/LocalFileSimulator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import jsLogger from "js-logger";
import { ILogger } from "js-logger";

import { VisDataFrame, VisDataMessage, TrajectoryFileInfoV2 } from "./types.js";
import {
VisDataFrame,
VisDataMessage,
TrajectoryFileInfoV2,
} from "../types.js";
import { ISimulator } from "./ISimulator.js";
import type { ISimulariumFile } from "./ISimulariumFile.js";
import type { ISimulariumFile } from "../ISimulariumFile.js";
import { LocalFileSimulatorParams } from "./types.js";
import { WebsocketClient } from "../WebsocketClient.js";

// a LocalFileSimulator is a ISimulator that plays back the contents of
// a drag-n-drop trajectory file (a ISimulariumFile object)
Expand All @@ -18,7 +24,14 @@ export class LocalFileSimulator implements ISimulator {
private playbackIntervalId = 0;
private currentPlaybackFrameIndex = 0;

public constructor(fileName: string, simulariumFile: ISimulariumFile) {
public constructor(params: LocalFileSimulatorParams) {
const { fileName, simulariumFile } = params;
if (!simulariumFile) {
throw new Error("LocalFileSimulator requires a ISimulariumFile");
}
if (!fileName) {
throw new Error("LocalFileSimulator requires a fileName");
}
this.fileName = fileName;
this.simulariumFile = simulariumFile;
this.logger = jsLogger.get("netconnection");
Expand Down Expand Up @@ -130,6 +143,10 @@ export class LocalFileSimulator implements ISimulator {
}
}

public getWebsocket(): WebsocketClient | null {
return null;
}

public getSimulariumFile(): ISimulariumFile {
return this.simulariumFile;
}
Expand Down
Loading
Loading