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

[Big change] Server refactor (Part 1) #858

Merged
merged 15 commits into from
Oct 19, 2022

Conversation

da-h
Copy link
Contributor

@da-h da-h commented Jul 29, 2022

Description

There has already been done lots of work regarding the refactoring and restructuring of the server code (see #675).
The goal of this PR is to merge these changes with the current server version to make room for further improvements on top of that. And most importantly: not loosing work in the long-run that has already been done.

(Note: Even though I tagged this PR as "Part 1", the changes suggested are still fully functional).

To accomplish the merge of #675

  1. First, I basically cherry-picked the commits containing the actual work (i.e. excluding the merge-commits). This has the side-effect that the original contribution is maintained in the logs.
  2. Then I've added revert-commits on top of that to essentially create a non-OP on-top of master.
  3. After that, I reapplied the refactoring applied in [Big change][WIP] Server refactor #675 and saved the changes as a new commit named reapply all changes since server refactor.
  4. Squashing that commit with the revert-commit basically results in a rebase of master onto [Big change][WIP] Server refactor #675.
    (The other way round would be possible as well, but I felt this was easier in this case).

Some minor changes on the way:

  • changed the method get_visdom_path to use importlib instead of inspect to get the actual file name of static files. (I found the previous version could have caching-problems with static files during development if I remember correctly).
  • Moved LAYOUT_FILE and MAX_SOCKET_WAIT to defaults.py (to match its location to that of the other global uppercased config variables).
  • Changed wildcard imports (from X import * to specific imports).

Note:
I needed to add a change in the github workflow just for this PR: As the file structure changed (the executable python file is py/visdom/server instead of py/visdom/server.py), the startup of the server would not work anymore for the "init"-stage of the cypress tests.
This (last) commit can basically be reverted after or removed before the merge.

How Has This Been Tested?

Just did a short test myself using the demo + tested against default cypress integration tests.

Types of changes

  • Refactor
  • 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)

Checklist:

  • I adapted the version number under py/visdom/VERSION according to Semantic Versioning
    Changed from 0.2.1 to 0.2.2.
  • 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
Copy link
Contributor Author

da-h commented Jul 29, 2022

Regarding the Socket-Wrappers & Handlers:
I have added a short explanation regarding the naming scheme of the socket wrappers (VisSocketWrapper, SocketHandler, ...) as code comments.
If I understood correctly, the VisSocket-Handlers are for clients that mainly write data, while the Socket-Handlers are for clients that mainly read data (for instance for visualization).

One question from my side: Why has this seperation been done during development?
Is it possibly an authentication reason or maybe a performance reason? I did not found any priorization or more performant connections for one or the other, but I didn't look too closely for such a feature.

I ask because I thought about merging these two types of handlers in case they are not actually handled differently.
I think this could simplify the code quite a bit.

@JackUrb
Copy link
Contributor

JackUrb commented Jul 29, 2022

Excited to see these changes revived!

You're right on the money for why there were different types of socket handlers. Back when we were driving internal visdom adoption we needed separate flows for writing to the server and reading from it for authentication reasons. The keepalive setups for each were different, and so we had to make a drop-in replacement for just one of the two paths. Nowadays we don't use this setup the same way anymore, so I'm happy to have things consolidated.

@da-h
Copy link
Contributor Author

da-h commented Jul 29, 2022

Ah I see, thanks for the additional information! :)

My tendency is not to simplify existing structures if they have a good reason like here. On the other hand, the code is indeed more complicated due to the split. Additionally, if nowadays more people need such a feature, probably a more complex role distribution (in contrast to just separating read and write) could become a long-term goal.

@da-h
Copy link
Contributor Author

da-h commented Sep 20, 2022

Rebased onto master to retrigger actions with the changes of #866 in place.

@da-h
Copy link
Contributor Author

da-h commented Oct 6, 2022

Not sure yet, how to handle version-numbers correctly, especially for multiple parallel open PRs, or the question if every small PR should actually increase the version.

Nevertheless, larger code changes such as this one could be given a new version number, i guess.
Thus, I have increased the version by a 0.0.1 step.

@da-h da-h force-pushed the server_code_refactor branch 4 times, most recently from e6a1e10 to 2989507 Compare October 6, 2022 22:56
@da-h
Copy link
Contributor Author

da-h commented Oct 6, 2022

I am sorry, did a mistake here in the "force-push" history by rebasing onto the wrong branch.

Last push sits correctly on top of master / v0.2.1.

@da-h
Copy link
Contributor Author

da-h commented Oct 17, 2022

(Just a rebase onto current master).

@da-h
Copy link
Contributor Author

da-h commented Oct 17, 2022

Was missing #873 PR.

Latest push contains that one as well. "Compare to previous Push"-Button should now show exactly those changes now.
(Did not see the conflict due to the whole file-restructuring of this rather large PR).

I am thinking what I could do to provide a clearer way to show if this PR is actually non-destructive somewhere. Possibly a script that reverts the split into sub-files?
Merging the files back programmatically to result in a file very similar to the current master's py/visdom/server.py should be possible.

@JackUrb
Copy link
Contributor

JackUrb commented Oct 17, 2022

If this is still functional as expected (which, given the tests, it likely is) I'm happy to accept this sooner rather than later (instead of building out a solution to constantly keep it updated).

@da-h
Copy link
Contributor Author

da-h commented Oct 17, 2022

Sure!
Just to mention it: the easier-to-review alternative is to split this up into two:

  1. Separating the changes that you have done in [Big change][WIP] Server refactor #675 plus the one commit afterwards reapply all changes since server refactor. This PR is the main work, structure-wise where it's much easier to have a script that checks differences to master by merging the separated files again.
  2. A second PR could contain the refactoring-approaches afterwards.

For both ways, merging this big PR, or, separating the two parts mentioned, I am happy to look into bugs that might have been introduced by me here.

@JackUrb
Copy link
Contributor

JackUrb commented Oct 17, 2022

I'll take a review pass tomorrow - given #675 was mostly me directly copy-pasting into smaller files and then making changes on top, I'm fairly convinced that the majority of the underlying server code is present as it was, so I'll do a review on the following content.

@da-h
Copy link
Contributor Author

da-h commented Oct 17, 2022

I didn't mean to push you. :)
A change of this size obviously needs some time to review.

@JackUrb
Copy link
Contributor

JackUrb commented Oct 17, 2022

No worries, I've left this one sitting open for far too long 😅

@da-h
Copy link
Contributor Author

da-h commented Oct 17, 2022

See a minor mistake that I will fix tomorrow morning: all the file headers say "Facebook Inc." instead of "Visdom Team". 😅

Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

All-in-all this is a great set of changes. Thanks for the additional work to extend my original set to still be valid against the current codebase. I feel like this separation does a lot to make the inner workings of Visdom more approachable.

@@ -1,5 +0,0 @@
# Copyright 2017-present, The Visdom Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we drop this file? The core function of it is to retain the directory, wherein people can import optional dependencies for Visdom. Realistically, this is only useful for the bhtsne library, but that may still be in use. Perhaps instead of just an __init__.py, it makes sense to have a README.md too, but that's out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't know that. I've removed the deletion in the latest push below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I'm not seeing the most recent push. I'll merge this in once it's there!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I totally forgot to push, but it should be there now.

@@ -6,6 +6,8 @@
# This source code is licensed under the license found in the
Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside, it's not the best that the entirety of visdom lives in the __init__.py file too. For a future refactor 😉

Copy link
Contributor Author

@da-h da-h Oct 19, 2022

Choose a reason for hiding this comment

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

Yeah, that's true; the python-client code grew out of control. ^^

Considering some of the issues, there are quite some bugs to fix there as well, but tons of tiny things to keep track of at the same time. Any Idea how to improve the code efficiently?

Copy link
Contributor

Choose a reason for hiding this comment

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

For things like this, I'd argue that visdom has reached a scale that we need to hit on more DevX and codebase quality. Before jumping into the small changes, reorganizing would come first. Then I'd probably want to start using typing, though that can happen in parallel with small changes (every time you take a pass through to fix a bug, doing code quality improvements in the same pass). Typing and testing make sure that as you're making changes, the code is to some degree keeping track of things by itself so you don't have to.

py/visdom/server/__main__.py Show resolved Hide resolved
py/visdom/utils/shared_utils.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
Comment on lines +53 to +66
# ============== #
# About & Naming #
# ============== #

# 1. *Handler- & *Wrap-classes are intended to have the same functionality
# - *Handler (e.g. VisSocketHandler) use WebSockets
# - *Wrap (e.g. VisSocketWrap) use polling-based connections instead
# - *Wrapper (e.g. VisSocketWrapper) is just a helper class for the respective *Wrap-class
# to process the current state (instead of the state at the time of polling)
# 2. VisSocket* classes (VisSocketHandler, VisSocketWrap & VisSocketWrapper)
# Their goal is to register clients with write access of actual data.
# 3. Socket* classes (SocketHandler, SocketWrap & SocketWrapper)
# Their goal is to register clients with read access of data.
# Write access is limited to data and view organization (i.e. layout settings, env removal and env saving)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for providing the demystifying details for these :)

Copy link
Contributor Author

@da-h da-h Oct 19, 2022

Choose a reason for hiding this comment

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

Yeah, wrapping my head around the code here was a bit tough. I felt that placing my notes here for others is surely helpful.

--

Maybe here is a good place to ask:
I am a bit unsure of how big of a PR one would like to have for changes like that, say, a refactoring of a large file. ;)

For instance, I do have worked a bit further on that file here; I merged the duplicated code as much as possible and placed the classes in a hierarchy, but felt that the change is altogether too much for one single PR.
Of course, larger code changes need more time to review, but on the other hand it does not make to much sense to split changes up too much, does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely too much for one PR. I think that a good PR can be large, so long as it's easily summarized. My rule of thumb is the simple sentence summary:
"Splitting up the server into related files, but retaining functionality" makes sense.
"Refactoring socket code to reduce duplicated code" also makes sense.
Putting these two together may be too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants