maxtext icon indicating copy to clipboard operation
maxtext copied to clipboard

[benchmarks] Fix xpk path to be `$HOME/xpk` rather than `maxtext/xpk`

Open SamuelMarks opened this issue 8 months ago • 4 comments

Description

Fix xpk path to be $HOME/xpk rather than maxtext/xpk

If the change fixes a bug or a Github issue, please include a link, e.g.,: FIXES: #1561

Nothing is wrong with ~ but you can't just use it like that. That's why we see maxtext/~/xpk rather than /home/myname/xpk

$ python3 -c 'import os; print(os.path.expanduser("~"))'
/Users/myname
$ python3 -c 'import os; print("~")' 
~

Details of os.path.expanduser: https://docs.python.org/3/library/os.path.html#os.path.expanduser

Tests

N/A

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • [x] I have performed a self-review of my code.
  • [x] I have necessary comments in my code, particularly in hard-to-understand areas.
  • [x] I have run end-to-end tests tests and provided workload links above if applicable.
  • [x] I have made or will make corresponding changes to the doc if needed.

SamuelMarks avatar Apr 17 '25 23:04 SamuelMarks

The existing code has a few assumptions about where xpk is installed. In some places, it expects it in ./~/xpk, in some, ./../xpk, in some ./xpk (in others, maybe even ~/xpk).

This PR is a big step in the right direction of consolidating them to ~/xpk but I am concerned some of our tests (and developers) rely on the current assumptions. I am not sure if the documentation clearly says where XPK should be installed but as part of this PR, we should make clear that XPK should be installed in the user's home directory.

Can we structure this in two parts, making a utility function in max_utils similar to the following?

def get_xpk_path(fallback_path=None):
  xpk_path = os.path.join(os.path.expanduser("~"), "xpk")
  if not os.path.isdir(xpk_path):
    if fallback_path is not None:
      max_logging.log(f"Could not find {xpk_path}. Falling back to {fallback_path}. Please install or move XPK to {xpk_path} because {fallback_path} will be DEPRECATED soon")
      xpk_path = fallback_path
    else:
      # error message
      ...
  return xpk_path

We can then communicate to developers and migrate tests before removing fallback_path and requiring XPK to be in ~/xpk.

lukebaumann avatar Apr 18 '25 17:04 lukebaumann

The existing code has a few assumptions about where xpk is installed. In some places, it expects it in ./~/xpk, in some, ./../xpk, in some ./xpk (in others, maybe even ~/xpk).

This PR is a big step in the right direction of consolidating them to ~/xpk but I am concerned some of our tests (and developers) rely on the current assumptions. I am not sure if the documentation clearly says where XPK should be installed but as part of this PR, we should make clear that XPK should be installed in the user's home directory.

Can we structure this in two parts, making a utility function in max_utils similar to the following?

def get_xpk_path(fallback_path=None):
  xpk_path = os.path.join(os.path.expanduser("~"), "xpk")
  if not os.path.isdir(xpk_path):
    if fallback_path is not None:
      max_logging.log(f"Could not find {xpk_path}. Falling back to {fallback_path}. Please install or move XPK to {xpk_path} because {fallback_path} will be DEPRECATED soon")
      xpk_path = fallback_path
    else:
      # error message
      ...
  return xpk_path

We can then communicate to developers and migrate tests before removing fallback_path and requiring XPK to be in ~/xpk.

Sure that's fine; as long as it's clear that this is a temporary solution that will be removed in a month or two… it's good not to let these things remain permanent otherwise you end up with a hodgepodge like glibc (which is why musl is sooooo nice).

I'll update this PR now.

SamuelMarks avatar Apr 21 '25 18:04 SamuelMarks

@bvandermoon

I basically just copied @lukebaumann code

Hmm, what I was thinking is to put all the user-set variables in one file. That's why I made these two files:

I would also like to remove all shell scripts and replace them with Python scripts. So having all parameterisation known in one location—well two for the test-specific one—and statically extendable would be especially useful for documentation; not to mention for fairly trivial: automated typed-CLI extrapolation.

(--xpk-path fallsback to XPK_PATH env-var fallsback to DEFAULT_XPK_PATH)

SamuelMarks avatar Apr 25 '25 17:04 SamuelMarks

This PR has been automatically marked as stale because it has not had recent activity. It will be closed soon if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Nov 09 '25 16:11 github-actions[bot]