Mention Java dependency in installation instructions

[tpanza]

📚 The doc issue

torchserve apparently requires Java (and, in particular, Java 17) in order to run. However, neither the README nor the Getting Started doc describe this requirement.

The closest I can find about this requirement are the Win Native and WSL docs. But anyone running on Linux would have no reason to read those docs. Furthermore, links to those Windows-related docs seem sparse.

Suggest a potential alternative/fix

Mention the need for Java to be installed, including the expected version of Java and setting the JAVA_HOME environment variable, as part of the high level installation and getting started instructions.

[msaroufim]

Today we do this via python ./ts_scripts/install_dependencies.py --cuda=cu102 which is mentioned in the README. Do you think it'd be best to briefly mention what this script does also in the main README?

[tpanza]

Thanks, @msaroufim Yes, I think the Java dependency is important enough to warrant mentioning up front in the README.

I suppose this is technically a separate issue, but the ts_scripts/install_dependencies.py does not appear to be a universal solution. For example, running apt-get install -y openjdk-17-jdk would not work for someone running on a RHEL-based distro.

[msaroufim]

Interesting, what did you need to do to make it work? yum? I'm happy to make install_dependencies more general for you

[tpanza]

Interesting, what did you need to do to make it work? I'm happy to make install_dependencies more general for you

For RHEL, this procedure is one way to install JDK. The package manager is yum, rather than apt-get. The package name is java-17-openjdk-devel, rather than openjdk-17-jdk.

Having a general Java installer sounds quite ambitious, IMO. Simply stating up front and explicitly in the docs that the users are required to install JDK17 would go a long way in the meantime. That way, they can take whatever custom steps are needed for their particular situation. The install_dependencies can be proposed as covering some, but not all, common scenarios.

As written, install_dependencies would benefit from renaming the current Linux class to DebianBasedLinuxDistro, DebianBasedLinuxDistroWithSudoPerms, or something like that.

There is another issue in that users in general will not necessarily have sudo privilege. Or if they do, they may not necessarily want to install the JDK17 at a system level.

One workaround to not having sudo is to install the JDK from conda-forge using mamba or conda. That assumes the user would have mamba or conda package managers installed and configured. If they did, though, it would be somewhat broader coverage than Debian-specific apt-get commands.

[msaroufim]

Interesting, lemme discuss this with @rohithkrn and get back to you - in the meantime if you'd like to make the PR to explain this better would be happy to merge it

Having a general Java installer sounds quite ambitious, IMO. Simply stating up front and explicitly in the docs that the users are required to install JDK17 would go a long way in the meantime. That way, they can take whatever custom steps are needed for their particular situation. The install_dependencies can be proposed as covering some, but not all, common scenarios.

As written, install_dependencies would benefit from renaming the current Linux class to DebianBasedLinuxDistro, DebianBasedLinuxDistroWithSudoPerms, or something like that.

[ozancaglayan]

I generally refrain from running dependency installer scripts unless I'm 100% sure what it does. So I think it'd be good to document all the steps performed in the install_deps also in the README file.

[tpanza]

@msaroufim would you accept a PR on this? A rough outline of my proposed changes:

• Update top-level README to mention the javac dependency (in addition to python >= 3.8)
• ts_scripts/install_dependencies.py updates:
  • Replace all os.system() calls with shutil.which() or subprocess.run()
  • Refactor install_java(): Rename to something like check_for_jdk() and make it behave similar to utils.check_python_version(). Check for the presence of javac on the PATH. If not found, exit with error status. Rationale:
    • The current Linux install_java() will fail if the user is not running a Debian/Ubuntu family of Linux distro that uses apt-get for package management. One could theoretically try to extend this by first doing something like this to determine the package manager, but this still would not work. This would require that users have (passwordless?) sudo/admin privileges. And even if they did, they might not necessarily want to perform system-level installs on their machines, just to run this Python library.
    • Some users may prefer/require to use Oracle JDK versus Open JDK. Having one flavor one forced on them (and especially a a sudo install at the system level) doesn't seem like a generalizable way to proceed. So, just throw an error and exit if javac not on the path.
  • A similar argument could be extended to the nodejs, node_packages, wget, and libgit2 installs, but that is probably left to a separate PR.

[msaroufim]

Hi @tpanza

• Replacing with subprocess.run sounds good
• install_java sounds good - the main thing to keep in mind is that we shouldn't break on the CI jobs so will need to update that job to make it work https://github.com/pytorch/serve/blob/master/.github/workflows/ci_gpu.yml#L41 and also need to make sure our docker builds still work https://github.com/pytorch/serve/blob/master/docker/Dockerfile.dev#L70
• Regarding nodejs and everything else I agree, we can leave to a seperate PR but I'd accept a PR doing all of htem in one go

Overall I'm very open to us having more friendly installation scripts but just need to make sure docs work, that beginners still have a script where everything just gets installed but also something that provides more flexibility to advanced users

We might just need to also document the force flag a bit better https://github.com/pytorch/serve/blob/master/ts_scripts/install_dependencies.py#L200 - it's something I added sometime back when I heard similar concerns

Regardless please send over the PR, @agunapal and I would be happy to review

[tpanza]

Hi @msaroufim

What do you think of the following approach? Replace the existing script install_dependencies.py into two:

• One used as part of DevOps automation, container builds, and torchserve developers. Here, we can safely assume we are on Ubuntu Linux, have sudo privilege, and apt-get is present
• One for basic end users of torchserve. This does the pip install of dependencies. Also checks for javac being present and throws an error if not. Perhaps this could even be incorporated into setup.py?

[CaseyHaralson]

Today we do this via python ./ts_scripts/install_dependencies.py --cuda=cu102 which is mentioned in the README. Do you think it'd be best to briefly mention what this script does also in the main README?

To me, the readme doesn't seem to suggest that running install_dependencies is necessary. That top block of the readme looks like it is giving options as to how to install torchserve: clone repo and install, install latest via pip, or install nightly via pip.