Skip to content

Merge in current hosting branch, which is our active server#147

Merged
davemfish merged 54 commits intonatcap:developfrom
dcdenu4:feature/vm-hosting
Mar 11, 2025
Merged

Merge in current hosting branch, which is our active server#147
davemfish merged 54 commits intonatcap:developfrom
dcdenu4:feature/vm-hosting

Conversation

@dcdenu4
Copy link
Member

@dcdenu4 dcdenu4 commented Feb 20, 2025

Mostly set up of an nginx proxy server.

dcdenu4 added 30 commits August 30, 2024 09:33
…non 443 ports. The endpoint 9000 is handled with a proxy pass to fileserver:9000
… caussing https ssl issues, so now 443 just does a proxy pass to localhost:8000
…hey were useful or not. Moving port 8000 and 9000 calls to 443 location proxy_pass redirects. Lots of SSL configuration and setup. Lots of things added to try and work and MixedContent errors and SSL erros.
…nginx. nginx when handling the request drops the trailing slash, which was causing a http redirect, which caused a https / http conflict
… cert issues. Commenting out return calls from GET / POST that were interfering with FastAPI returns, which caused a json error on the frontend.
@dcdenu4 dcdenu4 requested a review from davemfish February 20, 2025 15:52
Copy link
Contributor

@davemfish davemfish left a comment

Choose a reason for hiding this comment

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

Just a few misc things that caught my eye.

for output, output_path in carbon_outputs.items():
carbon_results[output] = pygeoprocessing.raster_reduce(
lambda total, block: total + numpy.sum(block), (output_path, 1), 0)
carbon_results[output] = f"{carbon_results[output]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, did something just break that required this? There's a json.dump(carbon_results) below. If there was something un-serializable in that data I wonder if it's a sign of a larger problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was seeing some breaking behavior in the logs of invest_results.py I hadn't before (that I can remember). It was reporting that 'float32' was not serializable. It looks like numpy.float32 is NOT serializable, but numpy.float64 is. It looks like, np.float64 is a subclass of float which is why it's serializable but np.float32 is not. We could also cast to float here, which is maybe what I'll do instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Casting to float here: 35bd170

WORKDIR /opt/frontend

ADD ./package.json /opt/frontend/
RUN yarn add puppeteer --ignore-scripts
Copy link
Contributor

Choose a reason for hiding this comment

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

puppeteer is already listed in the package.json, so I wouldn't expect to need to add it here everytime.

Copy link
Member Author

Choose a reason for hiding this comment

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

When running docker compose up –build the frontend container would hang indefinitely trying to build packages as part of yarn install.
It appears this was an issue with puppeteer, perhaps downloading Chromium
forever building fresh packages :( · Issue #5268 · yarnpkg/yarn · GitHub
Following the link above, a user suggested this, which worked right away for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know your thoughts on this and including yarn.lock. That puppeteer Chromium thing was a real blocker for me in a docker set up unfortunately.

"start": "vite",
"start-docker": "cross-env MODE=docker vite --host 0.0.0.0 --port 3000",
"start-docker-prod": "cross-env MODE=docker VITE_DEVMODE=false vite --host 0.0.0.0 --port 3000",
"start-docker-dev": "cross-env MODE=docker VITE_DEVMODE=true vite --host 0.0.0.0 --port 3000",
Copy link
Contributor

Choose a reason for hiding this comment

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

technically I think we're always using the vite dev server, even in production. So VITE_DEVMODE might be a bit misleading. Is it better as APP_DEV_MODE, or just DEVMODE?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is confusing, but I found I had to use VITE_ in conjunction with import_meta.env, otherwise the environment variable was not known.

To prevent accidentally leaking env variables to the client, only variables prefixed with VITE_ are exposed to your Vite-processed code.

So, we could rename it: VITE_UO_DEV_MODE or VITE_URBANONLINE_DEVMODE...

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to VITE_URBANONLINE_DEVMODE here: dca5e9e

Happy to change this to something else.

@@ -5,7 +5,9 @@ RUN mkdir /opt/frontend
WORKDIR /opt/frontend

ADD ./package.json /opt/frontend/
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should also be adding the yarn.lock file, to speedup the yarn install.

Copy link
Member Author

Choose a reason for hiding this comment

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

That could definitely be handy. See my note below on Chromium really slowing down / stopping the yarn install step when getting puppeteer.

#RUN yarn cache clean && yarn install --production=false

EXPOSE 3000

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's not used during docker compose, but the last (next) line in this Dockerfile could be updated to yarn start-docker-dev

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, or we just comment out / remove it all together. I'll update it for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed default CMD here: 18aaefb

@dcdenu4 dcdenu4 requested a review from davemfish March 10, 2025 14:28
@davemfish davemfish merged commit 6d00341 into natcap:develop Mar 11, 2025
1 check failed
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