Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: default to localhost on WSL #180

Merged
merged 8 commits into from Jul 4, 2023

Conversation

Lundez
Copy link
Contributor

@Lundez Lundez commented Jun 28, 2023

  • Make sure WSL works with default solara run (currently the $HOST is populated in WSL and we cannot open that browserpath from Windows...
  • Make sure we only reload the modules from CWD path

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.

  • Default to localhost if hostname is same as on WSL (.*?-w1\d)

@maartenbreddels
Copy link
Contributor

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 #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 #179 on the reload code where I take a slightly different approach (the directory we use there is the directory of the app.

@Lundez
Copy link
Contributor Author

Lundez commented Jun 28, 2023

@maartenbreddels Ok, should we close this PR then?

@maartenbreddels
Copy link
Contributor

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
Copy link
Contributor

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

@Lundez
Copy link
Contributor Author

Lundez commented Jun 28, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

Lundez commented Jun 28, 2023

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
Copy link
Contributor

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

@Lundez
Copy link
Contributor Author

Lundez commented Jun 28, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

Lundez commented Jun 28, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

Lundez commented Jun 29, 2023

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
Copy link
Contributor

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

@Lundez Lundez changed the title fix: Only reload necessary files & default to localhost on WSL fix: default to localhost on WSL Jul 1, 2023
@Lundez
Copy link
Contributor Author

Lundez commented Jul 1, 2023

@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
Copy link
Contributor

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
Copy link
Contributor Author

Lundez commented Jul 1, 2023

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.

solara/__main__.py Outdated Show resolved Hide resolved
Co-authored-by: Maarten Breddels <maartenbreddels@gmail.com>
@maartenbreddels maartenbreddels merged commit d483b87 into widgetti:master Jul 4, 2023
21 checks passed
@maartenbreddels
Copy link
Contributor

Awesome work @Lundez, thanks a lot!

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.

None yet

2 participants