Add docker and requirements.txt by Slyke · Pull Request #30 · rebane2001/matterport-dl

Slyke · 2022-01-06T00:14:42Z

No description provided.

Slyke · 2022-01-06T00:20:50Z

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.

Slyke · 2022-01-06T00:24:09Z

matterport-dl.py

 from http.server import HTTPServer, SimpleHTTPRequestHandler
 import decimal

+print = functools.partial(print, flush=True)


This is so that print flushes the output buffer. If this isn't set, print may not display anything until docker flushes the output buffer (and so the messages won't immediately appear when running in docker).

Slyke · 2022-01-06T00:25:56Z

matterport-dl.py


+print = functools.partial(print, flush=True)

+baseCloneDir = "clones/"


While not mandatory, it's probably a good idea to have dynamic files put into their own folder. This is mainly for docker to mount without issues, but also helps organise the project (also prevents accidental committing of clones and whatnots).

Only issue I see by setting this to a not-empty default is it breaks current users who have it in their root folder as they won't know it is looking in clones. It may be best to leave it empty and then have it be the docker default where it is guaranteed a new experience.

breaks current users who have it in their root folder as they won't know it is looking in clones.

Yep, it won't load those ones, but I do have an error message specifically for this case letting the user know to move it to the new clones/ directory.

I can still make it use the current dir, but it'll mess it a little bit because docker will run with a different directory than just the Python script, and this might confuse people.

Slyke · 2022-01-06T01:07:47Z

matterport-dl.py

        initiateDownload(pageId)
    elif len(sys.argv) == 4:
-        os.chdir(getPageId(pageId))
+        os.chdir(os.path.join(baseCloneDir, getPageId(pageId)))


I think this can be simplified to os.chdir(os.path.join(baseCloneDir, pageId)) from my experimentation as pageId is set above in the if block:

if len(sys.argv) > 1: pageId = getPageId(sys.argv[1])

I left it as is in case there is an edge case I'm not aware of.

mitchcapper · 2022-01-06T02:18:23Z

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

Switching the download folder to clones/ is a good idea, but a breaking change. A better option might be a --base_folder or similar command line switch to allow specifying a directory other than the current one to dl from.
You may be able to emulate this by simply calling the script from another folder, as it likely uses the terminals CWD as the root, but potentially not
In terms of it failing some ideas: try docker run -p 8080:8080 --entrypoint /bin/bash -t -i -v $(pwd)/clones:/matterport-dl/clones matterport_test python matterport-dl.py roWLLMMmPL8 127.0.0.1 8080 if that works then slowly remove the additions (interactive, tty, etc) until you find the issue. If that doesn't work just run bash and run the command by hand does that work?
I generally prefer CMD over entrypoint for dockerfiles I get the benefit here being to better support docker run but sometimes it may not do what people expect. The other option would be to use environmental variables instead (has the benefit of labeling too). IE docker run -p 8080:8080 -e MATTERPORT_ID=awerawer -e SERVER_PORT=1234 etc.

Slyke · 2022-01-06T08:14:10Z

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 · 2022-01-06T10:10:54Z

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 · 2022-01-06T10:12:18Z

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.

mitchcapper · 2022-01-07T00:19:20Z

matterport-dl.py


 def downloadAssets(base):
-    js_files = ["browser-check",
+    js_files = ["showcase","browser-check",


showcase was specifically removed as we must download it first and then manually parse it before proceeding.

Slyke · 2022-01-07T07:22:21Z

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 · 2022-01-14T10:47:37Z

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

Slyke · 2022-02-01T08:02:12Z

Just merged in the latest changes.

Is this going to be merged any time soon?

rebane2001 · 2022-02-01T08:14:31Z

@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 · 2022-02-01T09:54:03Z

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 · 2022-02-01T09:58:11Z

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 · 2022-02-02T23:33:39Z

Dockerfile

@@ -0,0 +1,11 @@
+FROM python:3.10.1-alpine3.14


broad versioning: FROM python:3

I had issues if 3:10 wasn't specified. Seems something is not compatible with tqdm (At least in Ubuntu). I did however remove alpine.

mitchcapper · 2022-02-02T23:34:43Z

README.md

+
+## Docker debugging
+```
+docker build --no-cache -t matterport-dl .


I am not sure you need --no-cache, at least in my experience there is no change the docker engine shouldn't detect and automatically rebuild with this buildfile.

I left the --no-cache in, in case someone that's not familiar with docker doesn't realise why their the code is not updating inside the docker image when running it. I can remove it, but it's only here for the debug.

mitchcapper · 2022-02-02T23:35:30Z

README.md

+## Docker debugging
+```
+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


With you shifting away from entrypoints that is not needed just adding "sh/bash" after "matterport-dl" will start that shell instead.

This is just for debugging. Can remove it, but it's so the developer is presented with a shell from the get go.

mitchcapper · 2022-02-02T23:41:03Z

matterport-dl.py

+        if os.path.isdir(pageId) and not os.path.isdir(os.path.join(baseCloneDir, pageId)):
+          print("The folder '" + pageId + "' does not exist in the '" + baseCloneDir + "' directory, but does exist in the root directory.")
+          print("If you recently updated matterport-dl, then please move the '" + pageId + "' folder into the '" + baseCloneDir + "' directory and run again")
+          sys.exit()
+        os.chdir(baseCloneDir)


My small quibble before was over breaking existing user current behavior. You detect and warn if this is the case, which was a great work around to warn users. After looking at it why not take it a slight step further: If we detect the pageId in the root dir, and baseCloneDir is set not specified we just don't change to baseCloneDir. I would do this for download and serve mode (incase the user is just resuming a download). This would 100% preserve existing behavior while also accomplishing the good default behavior you talked about for having a different clonedir by default.

You might be worried about once you serve multiple at once this not working well, but as that would be completely new functionality compatibility with root folder downloads would not be needed.

I left the warning in, but allowed it to use the folder if it's in the root directory. The warning waits for 2 seconds before continuing to load so that the user is aware something is not right. If this is not good, then please let me know!

mitchcapper · 2022-02-02T23:46:27Z

requirements.txt

+tqdm==4.62.3
+requests==2.26.0


broad versioning ie: tqdm>=4.62.3 or if one wanted to be super version safe tqdm >=4,<5

mitchcapper · 2022-02-02T23:55:04Z

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 · 2022-02-03T09:35:26Z

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

mitchcapper · 2022-02-03T11:39:00Z

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.

Added requirements.txt and Dockerfile Merge branch 'main' into add-docker Refactored dockerfile and created entrypoint script for using env vars Updated docker run examples. Refactored bootstrap code Added error for moving downloaded matterport to correct directory Remnoved show_case from js files, updated readme to run detatched mode added missing js files to aray Merge branch 'main' of github.com:rebane2001/matterport-dl into add-docker fixed uncaught merge conflict Made requested changes from PR

Slyke · 2022-02-04T11:33:42Z

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 · 2022-04-01T18:15:24Z

Hello, still waiting on this to be merged.

patricknelson · 2022-07-05T20:35:29Z

+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 · 2022-07-15T06:26:09Z

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

Slyke · 2022-07-17T17:49:36Z

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 · 2022-07-18T09:09:52Z

@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 · 2023-01-08T18:49:45Z

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 · 2023-06-12T00:09:20Z

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 · 2023-06-12T00:17:19Z

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.

Slyke · 2023-06-12T05:11:41Z

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

jdstone · 2023-06-19T00:02:23Z

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

Slyke commented Jan 6, 2022

View reviewed changes

Slyke mentioned this pull request Jan 6, 2022

requirements.txt #22

Closed

Slyke commented Jan 6, 2022

View reviewed changes

mitchcapper reviewed Jan 7, 2022

View reviewed changes

Slyke marked this pull request as ready for review January 7, 2022 07:20

rebane2001 mentioned this pull request Feb 1, 2022

Server - support serving multiple models at once #40

Closed

mitchcapper requested changes Feb 2, 2022

View reviewed changes

Slyke force-pushed the add-docker branch from d793807 to d031522 Compare February 3, 2022 09:38

Slyke force-pushed the add-docker branch from d031522 to 21a5dd6 Compare February 4, 2022 07:27

Slyke force-pushed the add-docker branch from 0198637 to 76540a0 Compare February 4, 2022 11:30

Merge branch 'main' into add-docker

97be3ad

Fixed shell to be bash

f56fcb9


		print = functools.partial(print, flush=True)

		baseCloneDir = "clones/"

		tqdm==4.62.3
		requests==2.26.0

Conversation

Slyke commented Jan 6, 2022

Uh oh!

Slyke commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchcapper commented Jan 6, 2022

Uh oh!

Slyke commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Slyke commented Jan 6, 2022

Uh oh!

mitchcapper commented Jan 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Slyke commented Jan 7, 2022

Uh oh!

Slyke commented Jan 14, 2022

Uh oh!

Slyke commented Feb 1, 2022

Uh oh!

rebane2001 commented Feb 1, 2022

Uh oh!

Slyke commented Feb 1, 2022

Uh oh!

rebane2001 commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Slyke Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchcapper commented Feb 2, 2022

Uh oh!

Slyke commented Feb 3, 2022

Uh oh!

mitchcapper commented Feb 3, 2022

Uh oh!

Slyke commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Slyke commented Apr 1, 2022

Uh oh!

patricknelson commented Jul 5, 2022

Uh oh!

mu-ramadan commented Jul 15, 2022

Uh oh!

Slyke commented Jul 17, 2022

Uh oh!

mu-ramadan commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Slyke commented Jan 6, 2022 •

edited

Loading

Slyke commented Jan 6, 2022 •

edited

Loading

Slyke Feb 3, 2022 •

edited

Loading

Slyke commented Feb 4, 2022 •

edited

Loading

mu-ramadan commented Jul 18, 2022 •

edited

Loading

jdstone commented Jun 12, 2023 •

edited

Loading