llm: results of chatgpt-5 agent tasked with improving readability and…#153
llm: results of chatgpt-5 agent tasked with improving readability and…#153brandongc wants to merge 1 commit intoNERSC:mainfrom
Conversation
… documentation (with diagrams)
| idx = 0 | ||
| for idx, item in enumerate(run_args): | ||
| image_index = 0 | ||
| for image_index, item in enumerate(run_args): |
There was a problem hiding this comment.
interesting, it missed the opportunity to excise the superfluous initialization of the index variable here. Also, arg_index seems like it would be a more appropriate expanded name.
swelborn
left a comment
swelborn
left a comment
There was a problem hiding this comment.
it seems like some exception behavior has changed. llms tend to go overboard catching generic exceptions unless you tell them not to, and in a lot of cases you want things to fail hard or handle in a different way
other changes with opening files and variable name changes seem reasonable
| - OSError: if attaching to the namespace fails. | ||
| """ | ||
| pth = f"/proc/{pid}/ns/{ns}" | ||
| tgt = open(pth, "r") |
There was a problem hiding this comment.
old version doesn't use context mgr - is not closing the file when this func goes out of scope intended?
| return | ||
| try: | ||
| subprocess.check_call(["mount", "--rbind", src, tgt]) | ||
| except subprocess.CalledProcessError as exc: |
There was a problem hiding this comment.
should this raise? it did before...
| copy_function=shutil.copyfile, | ||
| dirs_exist_ok=True, | ||
| ) | ||
| except OSError as exc: |
There was a problem hiding this comment.
should this raise? it did before...
| shutil.copyfile(src, tgt, follow_symlinks=(not symlinks)) | ||
| try: | ||
| shutil.copyfile(src, tgt, follow_symlinks=(not symlinks)) | ||
| except OSError as exc: |
There was a problem hiding this comment.
should this raise? it did before...
| Returns | ||
| - Dict keyed by module name containing config dictionaries | ||
| """ | ||
| try: |
| print("hook_tool must run as root (euid 0).", file=sys.stderr) | ||
| return 1 | ||
|
|
||
| try: |
There was a problem hiding this comment.
since certain exceptions get caught and don't raise here and in other places in main they will fail as a kind of warning rather than failing hard
| res.append(r) | ||
| done[id] = r | ||
| return res | ||
| def merge_records_preserve_first(record_lists, key_name): |
There was a problem hiding this comment.
would be nice if the agent type hinted these like it did in other parts of code
| if "graphroot" in line: | ||
| val = line.rstrip().split("=")[1] | ||
| store_path = val.replace(" ", "").replace('"', "") | ||
| except FileNotFoundError: |
There was a problem hiding this comment.
i would imagine this is not desired
| try: | ||
| tasks_per_node_match = re.search(conf.ntasks_pattern, tasks_per_node_raw) | ||
| ntasks = int(tasks_per_node_match[0]) if tasks_per_node_match else 1 | ||
| except Exception: |
| text=True, | ||
| ) | ||
| help_text = proc.stdout.splitlines() | ||
| except Exception: |
|
First of all thank you for spending your time looking at this LLM experiment! I'm going to close this PR, pick out some of the reasonable suggestions, and open some new focused PRs without the LLM. |
4 participants
This should perhaps not be merged as is. At least not without some careful consideration.
This is an untested (as of yet) experiment to see how a coding agent performs.
That said, several of the proposed changes appear to be improvements worth considering.