Added lambda that integrates Forklift#411
Added lambda that integrates Forklift#411JaimeZepeda08 wants to merge 10 commits intoopen-lambda:mainfrom
Conversation
tylerharter
left a comment
There was a problem hiding this comment.
Good work! Haven't read everything super closely yet, but leaving some initial feedback.
Let's not have big data files here, like deps.json and wl.json. In the docs, we can describe a user would use your lambda. wget those files (from wherever they are, probably on the forklift repo), and then curl to your function.
examples/forklift-service/f.py
Outdated
| event = json.loads(event) | ||
|
|
||
| workload_data = event.get("workload") | ||
| if not workload_data: |
There was a problem hiding this comment.
Do we need these checks? Seems it is all in a big try/except, and you'll return a similar error below anyway
examples/forklift-service/f.py
Outdated
|
|
||
| Expected event format: | ||
| { | ||
| "workload": { ... workload data ... }, |
There was a problem hiding this comment.
Can you show the next level of detail (below) about how workload data is structured?
examples/forklift-service/f.py
Outdated
| } | ||
| """ | ||
| try: | ||
| if isinstance(event, str): |
There was a problem hiding this comment.
Why do we need to check? Are there different possible inputs?
examples/forklift-service/f.py
Outdated
|
|
||
| result = generate_tree(workload_data, num_nodes, multi_package) | ||
|
|
||
| return { |
There was a problem hiding this comment.
Can we write a flask lambda instead? Then, we could return a status code, like 500 or something. The benefit is that somebody could do a "curl ... > tree.json" to get a tree, without needing to extract the result separately.
examples/forklift-service/f.py
Outdated
|
|
||
| # Testing code | ||
| if __name__ == "__main__": | ||
| with open("wl.json", "r") as file: |
examples/forklift-service/f.py
Outdated
|
|
||
| def generate_tree(workload_data, num_nodes, multi_package=True): | ||
| calls = parse_workload(workload_data) | ||
| deps_json = load_deps_json() |
There was a problem hiding this comment.
Let's not load from a file. Can we include include this in the POST body passed to our function?
examples/forklift-service/f.py
Outdated
| candidate_queue = [] # priority queue for candidate nodes | ||
|
|
||
|
|
||
| def enqueue_top_child_candidate(parent, deps, multi_package=True): |
There was a problem hiding this comment.
It seems we pass around deps and multi_package to all our functions. Perhaps put this all in a class, and these are attributes?
examples/forklift-service/f.py
Outdated
| continue | ||
|
|
||
| pkg_name = child_pkgV.split("==")[0] | ||
| version = child_pkgV.split("==")[1] if "==" in child_pkgV else None |
There was a problem hiding this comment.
Do we allow pkgs without a version? If our input is based on pip compile output, won't we have versions? I may misunderstand, though.
examples/forklift-service/f.py
Outdated
| continue | ||
|
|
||
| # keep track of packages that would be loaded by this candidate | ||
| packages_to_load = set([child_pkgV]) |
There was a problem hiding this comment.
Can create a set without converting:
{child_pkgV}
examples/forklift-service/f.py
Outdated
| from collections import defaultdict | ||
|
|
||
|
|
||
| # TODO: instead of loading deps from a file, use pip-compile on the fly |
There was a problem hiding this comment.
I'm thinking lets require callers to pass deps in. Then, however we figure out those dependencies, it will be outside the nice, clean tree-building logic here.
…ept deps as input, add paper link
| @@ -0,0 +1,290 @@ | |||
| ''' | |||
There was a problem hiding this comment.
just call "forklift", not "forklift-service"
| self._enqueue_top_child_candidate(child) | ||
|
|
||
| def build_tree(self, desired_nodes): | ||
| self.candidate_queue = [] |
There was a problem hiding this comment.
Best to see all class attrs in init even if None at that point.
For the queue, let's have namedtuple's so we can see the code is correct easier. Things like the following need careful checking:
_, _, best_candidate
| found := false | ||
| for _, p := range packages { | ||
| if p == nodePkg { | ||
| // compare only using package name |
| call_counts[name] += 1 | ||
|
|
||
| # build call matrix | ||
| calls = {} |
There was a problem hiding this comment.
is a dict the best type for a calls matrix? Do want something (a) sparse with (b) column names.
Need to check, but Claude says Pandas can have a pd.SparseDtype(int, fill_value=0) type to make it sparse. Maybe Pandas DF is the best matrix format then?
No description provided.