- 
                Notifications
    
You must be signed in to change notification settings  - Fork 418
 
Update XPK path to not include full package directory #1560
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few files with a similar issue, could you update them too ?
benchmarks/recipes/pw_long_running_recipe.py
benchmarks/recipes/pw_mcjax_benchmark_recipe.py
benchmarks/recipes/pw_mcjax_checkpoint_benchmark_recipe.py
          
 Thanks @shralex. I had pinged @shauryagup in the description on these files. I don't know how these files are used/I don't have a current Pathways setup in my environment to test that these changes work. So I was wanting the Pathways team to test it out on their side. Let me know if you think it's better to just make the changes here and then the Pathways team can check if they are working on their side later  | 
    
| 
           These can be updated in the same way. I will ping our chat space letting our team know of the change. pw_long_running_recipe.py  | 
    
          
 In fact, these should use the default value for   | 
    
| 
           There seems to be several updates needed for the Pathways benchmark recipes after the refactor. @SujeethJinesh will be addressing them in the coming days/weeks and will incorporate the changes to the XPK path in that refactor as well. TLDR the pathways benchmark recipes are broken and should not block this PR IMO.  | 
    
| 
           I tried making it   | 
    
| 
           Pathways tests will need a bit of a refactor that is coming, but we explicitly set the XPK path in our recipes. Changing the default shouldn't fix the pathways recipes regardless.  | 
    
fd6cbdf    to
    13f7722      
    Compare
  
    
          
 Thanks @lukebaumann . Went ahead and updated the XPK paths for the Pathways files. Pretty sure the previous path wouldn't work. @SujeethJinesh as FYI for when you go to refactor/get the files to actually work  | 
    
          
 @SamuelMarks not quite following this. I think it is just expecting the directory to be located in the home directory, right? That is how I have it setup on my local workspace and the XPK jobs kicks off successfully  | 
    
| 
           @bvandermoon If it works for your in CI that's great. Obviously it intends to expand to the user directory. Just saying it was flaky when I tried it on Monday… But if it works: merge! - Would like it to be properly referenced. EDIT: Quote #1561 (comment) 
  | 
    
Description
Use
~/xpk/xpk.pyas the defaultxpk_pathinstead of a path with the full package directory like/home/bvandermoon/workspace/maxtext/~/xpk/xpk.py. This change is needed to make the benchmark_runner work after a recent refactor.I was getting an error when running the benchmark runner without this change:
@shauryagup there are a few pathways files that look like they have this same issue. You may want to check if these files are still working for you and try this change if they are not:
Tests
The benchmark runner schedules the workload after this change and I get a link to logs in the XPK output.
Seeing a different issue now where the actual workload does not run properly. I will keep debugging that.
Checklist
Before submitting this PR, please make sure (put X in square brackets):