[benchmarks] Fix xpk path to be `$HOME/xpk` rather than `maxtext/xpk`
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 seemaxtext/~/xpkrather 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.
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.
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
~/xpkbut 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_utilssimilar 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_pathWe can then communicate to developers and migrate tests before removing
fallback_pathand 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.
@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-pathfallsback toXPK_PATHenv-var fallsback toDEFAULT_XPK_PATH)
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.