simonsobs / sisock

Sisock ('saɪsɒk): streaming of Simons Obs. data over websockets for quicklook
Other
2 stars 0 forks source link

Package Repo for pip Installation #8

Closed BrianJKoopman closed 5 years ago

BrianJKoopman commented 5 years ago

This pull request packages the sisock repo for installation with pip. It also organizes the servers into their own sub-directories within servers/.

The packaging allows for some improved behavior when setting up to run all the servers in Docker. I've also updated the Docker section in README.md to reflect these changes.

To summarize the changes on that end:

The installation of sisock, and packaging introduces a redundancy in the naming. This is evident in the updates for the sisock import, all of which have now become from sisock import sisock. This might not be desirable, feel free to suggest another name for sisock.py.

This resolves #7.

mhasself commented 5 years ago

I am quite troubled by from sisock import sisock. How about from sisock import base?

BrianJKoopman commented 5 years ago

Thanks for weighing in Matthew. I would lean towards renaming it and then changing all instance of sisock.* to sisock.base.*. Otherwise things become, for instance, base.DataNodeServer, which is less obvious to the reader that it's sisock related until you go read the imports. That is, unless you're okay with from sisock import base as sisock (which I suspect not.)

mhasself commented 5 years ago

Ya, I agree with you.

An alternative is to pull the most interesting and useful things into sisock.*, in the __init__.py. So __init__.py would have, e.g.:

from .base import DataNodeServer, other_stuff, this, that

I think that's fine, for critical objects in a relatively small library.

BrianJKoopman commented 5 years ago

Fixed. Now all imports are import sisock, and base is imported in the sisock __init__.py so that it's accessible via sisock.base.*.

Also created a Makefile which facilitates building the docker containers.

ahincks commented 5 years ago

The renaming looks sensible.

I'm not sure about having a single "servers" directory. The hub is notionally different from a data node server, as is the grafana server. They are all clients that connect to the same router but have different functions. (I think another term in WAMP-speak for a client is an "application component", but I can't readily find a glossary.) Currently we have three different types of client within the sisock universe that connect to the router:

Perhaps the directory structure could reflect this? I guess a longer-term question too is whether the data node servers all live in this repository or whether they are their own repositories.

I'm also open to reworking our terminology. E.g., I think "hub" works all right, but would it help if it were a little more descriptive? On the other hand, with "data node server", though a slight mouthful, there is little risk of confusion once you get the notion of "data node".

BrianJKoopman commented 5 years ago

I guess my thought process for the servers/ directory was more along the lines of, "these are all things that need to be run and remain running" (and all have an associated Docker image), rather than "these are all the same type of 'server'". My primary motivation being separating the components that go into Docker images to improve the build process on that end. I agree the items in there are not all the same, but I do think this is an improvement from keeping them all at the top of the repo directory structure.

Can you suggest something more concrete for how the directory structure should reflect these differences?

I'm not averse to breaking out the data node servers to a different repo. But I'm less convinced right now that they should all be in their own, since then the process of getting up and running is 'clone these 20 repos and build their associated images'.

All that said, the way they are organized right now is very modular. You could move these around however you want and I'd be reasonably confident in the rebuilding of them in Docker.

ahincks commented 5 years ago

How about the following directory structure?

components
└─ data_node_servers
     └─ server1
     └─ server2
     └─ …
└─ hub
└─ grafana_server
BrianJKoopman commented 5 years ago

Sounds good to me. I've pushed an amended commit with that structure and corrected documentation.

ahincks commented 5 years ago

Looks good, except that I think having the base directory called "servers" could be misleading—do you object to "components"?

BrianJKoopman commented 5 years ago

I think "servers" would be more familiar to those who haven't worked with WAMP before, but I don't object. I will change it to "components".

ahincks commented 5 years ago

Great, thanks. I just did a couple of tweaks in the README.md to reflect this change, and also have tested that it works on my system wtihout a hitch. I'm about to approve this pull request!