Skip to content
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

Optimize types/definitions build process #3692

Closed
wants to merge 1 commit into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 10, 2025

An attempt to try Claude after getting some confidence from @kentonv. Let's see if this change is worth 0.9$.

> /cost
  ⎿  Total cost:            $0.88
     Total duration (API):  9m 0.3s
     Total duration (wall): 14m 7.1s
     Total code changes:    28 lines added, 5 lines removed

Improve the build system to only rebuild types when source files change, rather than rebuilding for every build. This is accomplished by:

  1. Adding explicit file dependencies to the types target
  2. Creating a new build_types_when_needed target that tracks source files
  3. Adding a convenient alias in the root BUILD.bazel

Improve the build system to only rebuild types when source files change, rather than rebuilding for every build. This is accomplished by:

1. Adding explicit file dependencies to the types target
2. Creating a new build_types_when_needed target that tracks source files
3. Adding a convenient alias in the root BUILD.bazel
@anonrig anonrig requested review from a team as code owners March 10, 2025 21:01
@anonrig anonrig requested review from npaun and jp4a50 March 10, 2025 21:01
srcs = [
"scripts/config.capnp",
":types_worker",
"//:node_modules/prettier",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting... I think it is saying that if we bump the version of prettier it might change the types that need generating?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I think it should re-trigger since types generator calls prettier() javascript API directly.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suck it and see?

@anonrig
Copy link
Member Author

anonrig commented Mar 10, 2025

It actually doesn't build. I guess Claude didn't test the code before pushing...

@anonrig anonrig closed this Mar 10, 2025
@fhanau
Copy link
Contributor

fhanau commented Mar 11, 2025

I'm a bit bearish on LLMs in general, but based on how hard figuring out the nuances of Bazel is, we can't expect language models to do well here and not hallucinate things that are not what they seem to be at all:

  • The description of this change is highly misleading – the //types target is already sane and will not rebuild if its inputs do not change. Bazel has no way of knowing if types will change or not if there is a change to the workerd binary or to the other direct dependencies for type generation, there's no way around it.

  • A superfluous dependency on type definition files is added – this is already present in types_worker

  • The build_types_when_needed target is just a copy of types with insignificant changes like depending on additional files. It does not provide any benefit over types.

  • Adding the "manual" tag to types means that types will not run as part of //types/... – this might've be helpful if the target actually did what it is supposed to do, but by itself just means not generating types at all unless another target depends on them.

  • Subsequently, the alias in the top-level build file does not actually refer to a "Smart types target" at all.

@anonrig
Copy link
Member Author

anonrig commented Mar 11, 2025

So after some digging I found out that our types generation depends on jsg server. Which means whenever we change any file in this repository, type generation is rebuilt.

@fhanau
Copy link
Contributor

fhanau commented Mar 11, 2025

So after some digging I found out that our types generation depends on jsg server. Which means whenever we change any file in this repository, type generation is rebuilt.

Can you explain what you mean with "jsg server" and "any file in this repository"? Based on the direct dependencies of types, it depends on the workerd binary (//src/workerd/server:workerd), which is unsurprising as the type generation script invokes workerd. Thus any change to the workerd binary will require types to be regenerated (no way around that, unless there's e.g. whitespace-only changes that require workerd to be recompiled but result in the same binary/same hash which allows bazel to skip rebuilding dependents). But changes to e.g. src/workerd/server/server-test.c++ will not require generated types to be rebuilt as it is not among the set of dependencies for types.

@anonrig
Copy link
Member Author

anonrig commented Mar 11, 2025

I mistyped. You're right @fhanau.

@kentonv
Copy link
Member

kentonv commented Mar 12, 2025

I'm a bit bearish on LLMs in general

I was too until like two weeks ago. Claude Code is insane. See #3669 and #3673 for examples with transcripts.

But it's obviously not perfect. Seems like it didn't do a good job here.

I've been thinking about this a bit and I think we need a rule: No "vibe coding", that is, no submitting LLM-generated code where you don't really understand how or why it works. Draft PRs are OK if you want help figuring it out, but the PR must remain in draft until you understand it.

(Also please make sure you are following all company policies regarding both LLM use and also whether the particular software is approved for install on company machines. I made my two PRs on a personal machine out of caution. Of course, you can't put company code on personal machines unless it's open source.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants