Deduplicate Socket-Handler Code

[da-h]

Description

This PR merges duplicated code in the six classes from the file py/visdom/server/handers/socket_handlers.py:

• VisSocketHandler
• VisSocketWrapper
• SocketHandler
• SocketWrapper
• SocketWrap
• VisSocketWrap

reducing also the total file size by about 29%.

Notes:

• To be sure that no functionality is missing in the refactored code, this PR should not be merged before #896 that also tests for the polling variant of visdom.
• I probably did a lousy job at naming the classes:
  • First, I could not use the prefix Base as the code uses it already. Instead I used the prefix All.
  • Second, lacking a better unifying naming scheme for the *Handler and *Wrapper classes, I named the base class *HandlerOrWrapper.
  • Do you maybe have a better idea for a cleaner naming scheme?
• This also fixes a bug that probably no one has ever noticed: WebSocket clients could use the command pop_embeddings_pane, polling-based socket clients could not use the command. This PR fixes this bug implicitly.

Motivation and Context

Some duplicated code in the four classes has been quite entangled. This PR aims to reuse the respective code segments to make code changes more consistent.

How Has This Been Tested?

-

Screenshots (if appropriate):

-

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [x] code refactor or cleanup (changes to existing code for improved readability or performance)

Checklist:

• [ ] I adapted the version number under py/visdom/VERSION according to Semantic Versioning
• [x] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.

[da-h]

Suggestions

1. I propose a further unification by allowing any client to be both Sockets (i.e., reading data access) and VisSockets (i.e., writing data access). As both classes are function-wise completely orthogonal, the feature should be easily implementable in a backward-compatible way, and the change would simplify the code quite a bit.
2. Currently, the REST-API and the socket-API provide mostly orthogonal functions/commands to any client. For instance, the REST-API provides win_exists, fork_env, etc., while the socket-API provides save_layouts, delete_env, etc. I suggest merging the REST-API endpoints with the socket endpoints; both could offer the same commands to any client capable of handling these. Also, an easier-to-use endpoint structure could pave the way for further client implementations.

What do you think?

[JackUrb]

Currently, the REST-API and the socket-API provide mostly orthogonal functions/commands to any client.

This is an astute observation, and I agree that further unification would be possible. Perhaps this could be improved by a VisAPI base class, which could then be extended for versions using REST vs socket-based communication in either direction, and then people could initialize Visdom with configuration options that assist setup.

class VisAPI(...):
  ...

  @staticmethod
  def from_config(...) -> "VisAPI":
    ...

Then connecting clients to the server could be either, so long as they're using the agreed VisAPI.

[da-h]

This is an astute observation, and I agree that further unification would be possible. Perhaps this could be improved by a VisAPI base class, which could then be extended for versions using REST vs socket-based communication in either direction

I completely agree here!

and then people could initialize Visdom with configuration options that assist setup.

I am not sure if I understand it correctly. Do you mean: there could be a handshake between server and client that contains what commands they both agree with? So to speak a rolling API-version?

[JackUrb]

there could be a handshake between server and client that contains what commands they both agree with? So to speak a rolling API-version?

Ah not quite, that's more complicated than what I imagine. Just that the Visdom object can have different instances of the API abstraction to allow for all the weird network settings that exist out there, and so long as they adhere to the API (and either REST or sockets) then the server can register them

[da-h]

Then connecting clients to the server could be either, so long as they're using the agreed VisAPI.

One further question here: What would be the benefit of such an predefined agreement instead of just allowing any client to open any socket-type at any time? (The latter seems easier to implement & to manage, however, I am also open to implement the former if it is better.)

[JackUrb]

Then connecting clients to the server could be either, so long as they're using the agreed VisAPI.

One further question here: What would be the benefit of such an predefined agreement instead of just allowing any client to open any socket-type at any time? (The latter seems easier to implement & to manage, however, I am also open to implement the former if it is better.)

It's mostly just about establishing some common API layer, then we don't really need to care which one someone is opening.

[da-h]

Got it. I think we have simply misunderstood each other a bit here. :sweat_smile: Thanks for clarifying. :)

[da-h]

Rebased onto master to check if everything works with the new functional main-app. (Also applied the new black formatting rules).

[da-h]

Ok, some tests regarding moving an Image in the ImagePane fail. It seems the error is the same as in #901, i.e. the expected image dimensions are off by one or two pixels. Somehow the combination of #900 and #902 has changed the behavior of mouse-positions a bit.

I am converting this to a draft as well, until the test is fixed / the bug is found.

[da-h]

With #908 in-place, this PR succeeds all tests now & is thus now ready for review.