search

sparky8512 / starlink-grpc-tools

Random scripts and other bits for interacting with the SpaceX Starlink user terminal hardware
The Unlicense
482 stars 64 forks source link

Consider removing grpcurl / protoc commands from docker run time #23

Closed sparky8512 closed 3 years ago

sparky8512 commented 3 years ago

I mentioned this on issue #22, but then got busy with other things. While it's not directly related to that issue, I think whatever is decided on this topic may wind up resolving the performance issue reported in that one.

A few weeks ago, I implemented reflection support for the grpc scripts, so that the scripts would work without having to generate the pb2 protocol modules in advance. I did it as a separate Python package, though, and if that's not installed (via pip), the scripts will still attempt to load the generated pb2 modules out of the import path.

Right now, the new Python package (yagrc) is not being installed by the setup in Dockerfile, but that's OK because entrypoint.sh is still generating the pb2 modules via grpcurl and protoc. I think it would be better to switch to yagrc, though, as this would avoid a recurrence of what happened in issue #18.

I was testing some changes to Dockerfile and entrypoint.sh to do this by changing the pip install command to install everything from requirements.txt instead of listing the packages individually. However, after looking into what was going on in issue #22, I'm not so sure that's a good idea. The way Docker caches intermediate steps of the image generation makes it so that the image build can get "stuck" at whatever specific package versions were latest at the time that particular build step got cached. If that's going to happen anyway, then we should probably pin the packages being installed via pip in Dockerfile to specific known good package versions.

So anyway, I propose we drop the grpcurl and protoc commands from entrypoint.sh, add yagrc to the pip install command, and pin at least some of the packages to specific version numbers. grpcurl and grpcio-tools would no longer be required, so those could then be removed from the setup. Also, the package that seems to be causing at least part of issue #22, protobuf, is not even being installed directly, so to pin that to a known good version, that would also need to be added explicitly to the pip install command.

sparky8512 commented 3 years ago

@neurocis, any objections to what I proposed in the last paragraph of the prior comment? If not, I'll work it up in a branch and have you review the PR. I've been a bit distracted the past few weeks, but I should have some time to spend on this project again now, and I'd like to get this wrapped up.

neurocis commented 3 years ago

No objection, sounds like a great idea. grpcurl seemed like a bit of a hacked together deployment into the container anyhow.

sparky8512 commented 3 years ago

This change has now been merged into the main branch. @neurocis, could you please update the published neurocis/starlink-grpc-tools image with this config change when you get a chance?

neurocis commented 3 years ago

Hub build in progress now.

sparky8512 commented 3 years ago

Thanks. I have confirmed that the updated image is available to pull and has the expected packages/versions. That should wrap up this issue.