rebane2001 / matterport-dl

A downloader for matterport virtual tours
The Unlicense
322 stars 79 forks source link

Add docker and requirements.txt #30

Open Slyke opened 2 years ago

Slyke commented 2 years ago

Hey @mitchcapper having some issues with the static python webserver.

I've created a new directory called clones for all the downloaded clones to go into. Currently it's downloading correctly into this folder, at least as far as I can tell.

But when running the webserver with this new directory (docker run -p 8080:8080 -v $(pwd)/clones:/matterport-dl/clones matterport-dl roWLLMMmPL8 127.0.0.1 8080) Chrome is complaining ERR_EMPTY_RESPONSE and I believe it's due to not correctly getting the files from the clones directory.

Adding in:

    def __init__(self, *args, **kwargs):
        SERVEDIR=os.path.join(baseCloneDir, pageId)
        super().__init__(*args, directory=SERVEDIR, **kwargs)
        print('SERVEDIR', SERVEDIR)

Just below class OurSimpleHTTPRequestHandler(SimpleHTTPRequestHandler): doesn't seem to do anything, so I'm guessing the server isn't starting correctly?

If you can please test this on OSX before merging in that'd be helpful too. I have tested this on Windows 11 running Docker Desktop WSL2 and apart from the issue I just mentioned, it seems to be working fine.

I may submit a follow up PR after this one is merged that returns a JSON array of current clones from the clones directory if an ID is not specified in the main route and allow for serving all IDs listed in the clones directory.

mitchcapper commented 2 years ago

Thanks for the PR docker support is always great. A few items of note:

Slyke commented 2 years ago

Yep, I'll give that a try. I'll start it shortly.

I just did the other refactorings. The reason I want to make it use clones (or another sub folder) by default is so that the Python webserver can serve all of the downloaded matterports. Ie, it won't be necessary to specify the matterport ID when starting the server.

Edit: Figured it out. It was a docker networking thing. Had to bind to 0.0.0.0. Still not ready for merge.

Slyke commented 2 years ago

Okay, I think that's it. I'll give it a thorough testing tomorrow sometime. I added an error message explaining to move the matterport IDs into the clones/ directory if it's in the main directory. If it doesn't exist in either directory, it'll error stating to download it. So far my smoke tests are good. Let me know if you're happy with the changes.

I'll do the upgrade for allowing multiple matterports to run at the same time in another PR, along with creating the JSON array of IDs and maybe a basic HTML page that lists all the existing IDs, if an ID is not passed in.

mitchcapper commented 2 years ago

Ah well to serve multiple at once is a scenario it was not designed for. There is a lot of root / path expectations. you could probably change the startswith to contains and not have much conflict (regex's would be more accurate but probably worse performing, would need testing though to be sure). You will also need to update openDirReadGraphReqs to support getting multiple id's graph data from the local drive.

One option might be to make one handler per ID, with then a super handler that hands off to the individual handlers after stripping the leading model ID from the request. You could pass the graph data then to the handlers for that individual ID rather than having it global.

If you only have a few ID's you plan to host you could always just run a docker instance per as well.

Slyke commented 2 years ago

All my testing seems fine. It runs with and without docker just fine. I didn't try the PROXY flag inside docker though. This should be tested on OSX with docker, and on the bare metal.

Slyke commented 2 years ago

I updated the PR with the new JS files from matterport.

Slyke commented 2 years ago

Just merged in the latest changes.

Is this going to be merged any time soon?

rebane2001 commented 2 years ago

@mitchcapper

Ah well to serve multiple at once is a scenario it was not designed for. There is a lot of root / path expectations.

While out of scope of this PR, I think this might be a great feature to add in general, might even implement a custom index page that lists all the downloaded places so you can pick from them.

The path problem could be solved by looking at the GET parameter (?m=something) in the URL or the referrer. There are a few requests without either, but for those we could just look at what the most recent id was.

Slyke commented 2 years ago

While out of scope of this PR, I think this might be a great feature to add in general, might even implement a custom index page that lists all the downloaded places so you can pick from them.

Yup! That's exactly what I plan to do. Basically a JSON array or a basic HTML output/gallery view. Wanted to get this merged in first to keep the PRs smaller.

Using a query param would work too, but I think I can make it work with something like localhost:8080/roWLLMMmPL8 or localhost:8080/?m=roWLLMMmPL8 fairly easily. Using the most recent id might present some problems though if there were ever 2 people using the webserver loading different IDs, or if it's running behind a load balancer with multiple copies of the pod running.

rebane2001 commented 2 years ago

The few requests that don't have the referer header are rare, most do have the header and I don't think that approach would cause any issues.

mitchcapper commented 2 years ago

Some minor additional comments to consider. The broadened the version requirements just to increase the chance of security/bug fixes not being missed due to a locked in version. The support for both cloneDir and existing non cloneDir items is up to @rebane2001 how much they care about potentially breaking existing downloads without the user re-arranging. You did 95% of the work though to support it so figured I would mention that option.

You may also want to squash and merge when merging this PR for a cleaner history.

Slyke commented 2 years ago

If everything's good, I'll squash the commits so the PR can be merged.

mitchcapper commented 2 years ago

lgtm

two things totally don't need changing but curious if im missing something:

docker build --no-cache -t matterport-dl .
docker run --entrypoint /bin/sh -t -i -p 8080:8080 -v $(pwd)/clones:/matterport-dl/clones -e M_ID=roWLLMMmPL8 -e BIND_IP=0.0.0.0 matterport-dl

so for --no-cache I believe: if you change the local python file even without that flag it should automatically update. Docker sends your local items to the server for the build and if they change rebuilds automatically any steps that need to be rebuilt.

Does entrypoint actually work after the other change? As you changed CMD to a script maybe it uses /bin/sh as the entrypoint(as that is the default anyway) and still runs the docker_entrypoint.sh script (but didn't actually test). I was saying for the debug command more of: docker run -t -i -p 8080:8080 -v $(pwd)/clones:/matterport-dl/clones -e M_ID=roWLLMMmPL8 -e BIND_IP=0.0.0.0 matterport-dl /bin/bash

Again no need to change these things as won't matter to anyone, but that is my understanding. You are right about the python version, although I think your pip requirements should force it to update to a new enough version to not throw an error. I am also surprised the python:3 tag would pull an older version (although more likely you have an older version in your local cache as that does not auto refresh). This could happen to other users too given how common of a base image it may be.

Slyke commented 2 years ago

so for --no-cache I believe: if you change the local python file even without that flag it should automatically update. Docker sends your local items to the server for the build and if they change rebuilds automatically any steps that need to be rebuilt.

Yep, most of this time this is true. Sometimes, for whatever reason though, Docker refuses to detect changes. This forces it to rebuild. This can be really annoying when trying to debug something, and it not updating your code and you don't realise it.

I was saying for the debug command more of

Ohh I got you now. Yup, no reason. I changed it to the way you suggested.

I squashed all my commits into one commit and left the merge commit in too.

Please do one final test before merging! But all is good my end.

Slyke commented 2 years ago

Hello, still waiting on this to be merged.

patricknelson commented 2 years ago

+1 on merging this. I stumbled up on #42 since following README.md didn't work for me (I'm not a python hacker... yet...). I've actually got it running in Docker so this would probably be super useful for someone a bit more entry level to get started 😊

mu-ramadan commented 2 years ago

Hi @Slyke does the script supports serving multiple tours at the same time ?

Slyke commented 2 years ago

Hi @Slyke does the script supports serving multiple tours at the same time ?

No, I'll make it do that once this has been merged in. This PR is already quite large and it's been waiting for 6+ months, so don't want to make anymore changes unless necessary.

mu-ramadan commented 2 years ago

@Slyke it will be great if you update it now since the main repository is not updated for more than 7 months, it's not even working anymore. I've tried to temporary fix #66 the current issues it's working fine but I need to serve multiple tours I can't figure out how to do that, always getting 403 error. if you already have the updated script to serve multiped tours please send it to me at mu.ramadan@outlook.com

ideasxiang commented 1 year ago

Hi @Slyke , I am planning to write a script to support multiple tours at the same time. Is there any downside to running multiple docker instances at the same time?

jdstone commented 1 year ago

I had successfully created my own Docker image, but when I run it, sometimes it serves the model and other times it does not. No other errors are produced except for a lot in the browser developer console. Mainly CORS error's. I tried modifying the code to send a 'Access-Control-Allow-Origin', '*', but that did not work. So I thought I'd try this.

I tried this code and successfully created a Docker image. But when it runs and I browse http://localhost:8080 in the browser, it just says connection refused.

Any ideas?

jdstone commented 1 year ago

I tried this code and successfully created a Docker image. But when it runs and I browse http://localhost:8080 in the browser, it just says connection refused.

Duh, I didn't add -p to the docker run command. But now it just says "Oops, model not available" -- it doesn't look like it's calling all the correct network requests.

I'm going to play with it.

image

image

Slyke commented 1 year ago

I don't have the ability to merge. I'm just a random contributor too. image

jdstone commented 1 year ago

@Slyke, I know. I was just asking for some help.