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

Documentation: Host sample updated for IPv4+IPv6. #5372

Closed
wants to merge 1 commit into from

Conversation

jboero
Copy link
Contributor

@jboero jboero commented Feb 6, 2024

Simple note in case anybody needs IPv6.

In case anybody needs IPv6.
@@ -20,7 +20,7 @@ Command line options:
- `--lora FNAME`: Apply a LoRA (Low-Rank Adaptation) adapter to the model (implies --no-mmap). This allows you to adapt the pretrained model to specific tasks or domains.
- `--lora-base FNAME`: Optional model to use as a base for the layers modified by the LoRA adapter. This flag is used in conjunction with the `--lora` flag, and specifies the base model for the adaptation.
- `-to N`, `--timeout N`: Server read/write timeout in seconds. Default `600`.
- `--host`: Set the hostname or ip address to listen. Default `127.0.0.1`.
- `--host`: Set the hostname or ip address to listen. Default `127.0.0.1`. Use `::` for any IPv4+IPv6.
Copy link
Collaborator

@cebtenzzre cebtenzzre Feb 6, 2024

Choose a reason for hiding this comment

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

I think this addition would need to provide at least a little more context.

  • 127.0.0.1 is the IPv4 localhost address, 0.0.0.0 is the IPv4 wildcard address.
  • ::1 is the IPv6 localhost address.
  • 'localhost' should resolve to both IPv4 and IPv6 addresses, on a properly configured system.
  • :: is the IPv6 wildcard address, and its meaning depends on the OS - on Windows it binds to IPv6 only. On Linux it will also bind to IPv4 if sysctl net.ipv6.bindv6only is zero, which is usually the case. I'm not sure about macOS.

I think there would need to be a code change to reliably listen on both stacks on any OS. If configured correctly, providing the hostname of the local host instead of :: is probably sufficient - but I don't know how e.g. a Windows system will default to resolving its own hostname (e.g. it could give 127.0.0.1 instead of the local IPv4 address).

Copy link
Contributor Author

@jboero jboero Feb 6, 2024

Choose a reason for hiding this comment

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

What change? Try it. It works on both stacks already. At least for me in Linux it works for both IPv4 and IPv6 as standard. That works for Go apps also. Oh I see what you mean now.

I agree it could use more context maybe but the functionality is there.

@cebtenzzre
Copy link
Collaborator

I ran into this today when trying to run the server tests (which assume the server is listening on IPv4). The root issue here is that in create_socket, httplib is calling getaddrinfo and then binding to only the first address family in the list, instead of creating a socket for each supported address family like many programs do.

@slaren
Copy link
Collaborator

slaren commented Mar 19, 2024

The version of httplib that we are using is very outdated (0.12.2), there have been a lot of fixes since this version, including security issues. It should probably be updated.
https://github.com/yhirose/cpp-httplib/releases

@phymbert
Copy link
Collaborator

I think we should add an option to restrict the IP family in the bind socket flags in the server:

inline bool Server::bind_to_port(const std::string &host, int port,
int socket_flags) {
return bind_internal(host, port, socket_flags) >= 0;
}

In:

https://github.com/ggerganov/llama.cpp/blob/master/examples/server/server.cpp#L2855-L2858

See:

@mofosyne mofosyne added documentation Improvements or additions to documentation Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix labels May 9, 2024
@mofosyne mofosyne added the obsolete? Marker for potentially obsolete PR label May 17, 2024
@mofosyne
Copy link
Collaborator

mofosyne commented May 17, 2024

Seems like these changes are already in the main branch. Obsolete PR?

(Btw, use https://docs.github.com/en/actions/managing-workflow-runs/skipping-workflow-runs to skip documentation only PR ci checks)

@jboero jboero closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation obsolete? Marker for potentially obsolete PR Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix server/webui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants