-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add Galley option to compute() #79
Conversation
All tests pass locally if you point to the Finch main branch. |
Example Script:
Galley Exec Time: 0.4668142795562744 |
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.
Overall, changes LGTM!
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.
This looks good, but I'd like to make the galley optimizer and the default optimizer be separate functions, so that we can pass them different options. does that make sense?
src/finch/compiled.py
Outdated
jl.Finch.set_scheduler_b(jl.Finch.galley_scheduler()) | ||
return | ||
|
||
def clear_optimizer_cache(): |
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.
This should not be necessary with the latest update (Finch 1.0.0)
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.
Do you mean the separate functions or the clearing of the cache?
I just checked and the cache is still being hit without the clearing.
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.
Have you pulled the latest finch? I just want to double check. It was my intention that Finch 1.0.0 would clear the cache if you use a different tag, but not if you use the same tag. the default does not clear the cache,
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.
if you want to clear it every time, you may want to give it a different tag every time
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.
I am not sure if I will be able to clear the cache efficiently in future versions
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.
Ah, I see. I need to add tag parameters to this then. That makes sense. I'll update it with that.
Let's also make this PR one that moves to 1.0.0 Finch version (but I guess this is required here anyway). |
Also, in |
Pending @willow-ahrens final review, I think this is good to merge! |
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.
a few very minor points.
|
||
class DefaultScheduler(AbstractScheduler): | ||
def __init__(self, verbose=False): | ||
self.verbose=verbose |
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.
let's put get_julia_scheduler inside the classes, so that we can pass different flags. Then we can just call opt.get_julia_scheduler() in compute.
src/finch/compiled.py
Outdated
self.verbose=verbose | ||
|
||
def get_julia_scheduler(opt): | ||
if isinstance(opt, DefaultScheduler): |
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.
I'm just commenting that the isinstance checks can be avoided here, and make it more modular to change just one scheduler or extend in the future
|
||
def compute(tensor: Tensor, *, verbose: bool = False): | ||
def compute(tensor: Tensor, *, verbose: bool = False, opt=None, tag=-1): |
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.
I don't know if tag=-1 is respected by Finch anymore, it will just be a tag named "-1" without a sentinel behavior.
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.
That's fine I guess. It can just be the default tag, and programs can be cached unless the user makes a new tag.
This PR enables galley optimization through the python interface in two ways 1) as a parameter to compute 2) by setting the global scheduler with set_optimizer.