widgetti / solara

A Pure Python, React-style Framework for Scaling Your Jupyter and Web Apps
https://solara.dev
MIT License
1.62k stars 105 forks source link

fix: default to localhost on WSL #180

Closed Lundez closed 10 months ago

Lundez commented 10 months ago

Q: why don't we always simply default into localhost? To me it makes sense to use localhost all the time unless specified in --host.

maartenbreddels commented 10 months ago

Hi,

thanks for the PR! It is quite common to use HOST, but I have to say that I get quite annoyed by it as well, in conda environment it can also be set. Note that I am currently changing this code in https://github.com/widgetti/solara/pull/167 as well and we could say we only use SOLARA_HOST, instead of HOST to get rid of these annoyances.

I'm working in https://github.com/widgetti/solara/pull/179 on the reload code where I take a slightly different approach (the directory we use there is the directory of the app.

Lundez commented 10 months ago

@maartenbreddels Ok, should we close this PR then?

maartenbreddels commented 10 months ago

Yeah, I think we can close this, but we could integrate your HOST workaround in that PR as well, I'll cherry-pick it

maartenbreddels commented 10 months ago

Actually, why is your HOST env var set? is it also due to conda?

Lundez commented 10 months ago

Actually, why is your HOST env var set? is it also due to conda?

This is done by WSL I believe... I first thought I couldn't run solara using WSL (pass-through the firewall). Streamlit worked and I noticed that solara defaulted to a weird <username>-w11. Changing --host to localhost all worked fine!

I use micromamba, could be that, but based on the $HOST=<username>-w11 I don't think so.

maartenbreddels commented 10 months ago

could we change it to also ignore specifically a hostname ending on w11 on windows platforms? (e.g. if sys.platform == ''win32'' and DEFAULT_HOST.endswith("-w11"). Otherwise we might introduce a breaking change.

Lundez commented 10 months ago

could we change it to also ignore specifically a hostname ending on w11 on windows platforms? (e.g. if sys.platform == ''win32'' and DEFAULT_HOST.endswith("-w11"). Otherwise we might introduce a breaking change.

I found online that the safest way to do this was through uname() as I did. Because platform is a Linux distribution when you use WSL..

maartenbreddels commented 10 months ago

ok, so it's "microsoft-standard" in uname().release and DEFAULT_HOST.endswith("-w11") ? Can you confirm that that evaluates to true?

Lundez commented 10 months ago

ok, so it's "microsoft-standard" in uname().release and DEFAULT_HOST.endswith("-w11") ? Can you confirm that that evaluates to true?

>>> from platform import uname
>>> uname()
uname_result(system='Linux', node='hlondogard-w11', release='5.15.90.1-microsoft-standard-WSL2', version='#1 SMP Fri Jan 27 02:56:13 UTC 2023', machine='x86_64')
...
>>> import os
>>> os.environ.get("HOST")
'hlondogard-w11'

Is my current "automatic WSL" setup. This is true even outside a micromamba (conda) environment. I'd recommend only looking at release, because Windows 10 should probably have w10 suffix or something, right?

maartenbreddels commented 10 months ago

But I don't want to disable HOST completely, since it is a common way to configure the domain/ip to bind the socket to.

Lundez commented 10 months ago

But I don't want to disable HOST completely, since it is a common way to configure the domain/ip to bind the socket to.

Sure, I simply recommend defaulting to either:

  1. localhost unless --host is used (you could do solara run --host $HOST app.py

Or

  1. Use uname().release to validate if wsl and then default to localhost unless $HOST is used.

Personally I haven't seen a tool previously default into $HOST. My expectation would be localhost or 127... unless I explicitly set it. But that's just my experience from Python/Java/Kotlin/Scala.

maartenbreddels commented 10 months ago

I'm not sure I understand you. solara run --host $HOST app.py will expand to whatever $HOST is by the shell. Solara does not know you are using the $HOST env var, it will simply get the value of it. So I do not know the intent of the user, the intent is normally, if $HOST is set, use it.

FLASK does it, and the nodejs servers I used, like the one that comes with nuxt. I often find out, because also my HOST variable is set to something invalid :)

Lundez commented 10 months ago

I'm not sure I understand you. solara run --host $HOST app.py will expand to whatever $HOST is by the shell. Solara does not know you are using the $HOST env var, it will simply get the value of it. So I do not know the intent of the user, the intent is normally, if $HOST is set, use it.

FLASK does it, and the nodejs servers I used, like the one that comes with nuxt. I often find out, because also my HOST variable is set to something invalid :)

Okey! I understand where you're coming from. I've mainly seen changing host as a code or config option. But if it's default in flask I guess it could make sense.

With that said, I was quite confused when starting solara and couldn't get it to work. All because it defaults into $HOST which WSL had set without my knowledge. I would've expected it to work like streamlit, which default into localhost unless a config has been set.

I think a compromise could be to default with $HOST as you do, but if it's used then warn/inform through the terminal that it's used. Is that sensible? E.g. solars is running on XYZ ($HOST) if that's used.

maartenbreddels commented 10 months ago

Now that #167 is in, could you rebash this (or force push) with a fix to ignore HOST with (.*)-w1[0-9]

Lundez commented 10 months ago

@maartenbreddels what's the default here, a squash and merge? If so the commit-history doesn't matter. Else I'll force-push a prettier history.

Please review changes.

maartenbreddels commented 10 months ago

Depends, in any case master should be clean. So if a pr is a single change, it should become 1 commit. If you don’t force push I’ll squash merge. If you keep it clean and force push, also good. But 1 pr with multiple features (I’m ok with that is the build on each other) just force push and rebase. We try to stay close to conventional commits.

Lundez commented 10 months ago

Depends, in any case master should be clean. So if a pr is a single change, it should become 1 commit. If you don’t force push I’ll squash merge. If you keep it clean and force push, also good. But 1 pr with multiple features (I’m ok with that is the build on each other) just force push and rebase. We try to stay close to conventional commits.

Let's squash and merge, giving the new commit the title from the PR. That's usually how I do it.

maartenbreddels commented 10 months ago

Awesome work @Lundez, thanks a lot!