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

Move override segment suffixing from planning time to task instantiation time #4176

Open
lihaoyi opened this issue Dec 24, 2024 · 2 comments · Fixed by #4179 · May be fixed by #4181
Open

Move override segment suffixing from planning time to task instantiation time #4176

lihaoyi opened this issue Dec 24, 2024 · 2 comments · Fixed by #4179 · May be fixed by #4181
Milestone

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Dec 24, 2024

example.thirdparty[gatling].testRepoRoot seems to invalidate unnecessarily when running ./mill -w docs.fastPages and editing example/large/selective/9-selective-execution/build.mill

The problem appears to come from the #1600, where we made tasks get labelled and assigned .super.foo suffixes during planning. Thus, depending on what exactly Mill is asked to evaluate, the overridden example.thirdparty[gatling].testRepoRoot in the preparatory evaluation has the same label as the overridingexample.thirdparty[gatling].testRepoRoot in the main evaluation. This causes them to collide on disk and results in the latter unnecessarily invalidating since the hashes appear to have changed (since the other task stomped over the json files on disk)

Even outside of --watch, this scenario can probably occur when running two different sets of tasks manually:

  • An overridden task may have a .super suffix and the override task no .super suffix in the first run
  • The overridden task has .super suffix with the override not evaluated in the second run the other
  • Thus the override from the first run would share the overridden task segments/out-folder in the second run, potentially causing spurious invalidations since the two tasks will have different sets of inputs and input hashes

This whole "tasks are assigned their final label during planning" thing is an endless source of bugs, and was originally introduced because we started using the ordering of tasks during planning to determine override ordering which is necessary to assign .super.Foo suffixes as part of the labels in (#1600). Although that PR fixed stackable traits overrides from colliding in the same evaluation run, it introduced this issue which can cause overriden and overriding tasks to collide across multiple evaluation runs

We probably should try to detect move override detection and label allocation to task instantiation time to avoid this footgun.

@lihaoyi lihaoyi reopened this Dec 25, 2024
@lihaoyi lihaoyi changed the title Unexpected invalidation when rendering Mill documentation Overriding a Task.Source with a Task causes unnecessary invalidation during ./mill --watch Dec 25, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 25, 2024

Might need to go into Mill 0.13.0 if we need to add more implicit macro parameters to the Task constructors, to capture the class that they were defined in so we can decide which one is the override and which ones are overridden.

Could possibly re-use some of the refactoring necessary in #4026

@lihaoyi lihaoyi changed the title Overriding a Task.Source with a Task causes unnecessary invalidation during ./mill --watch Move override segment suffixing from planning time to task instantiation time Dec 25, 2024
@lihaoyi lihaoyi added this to the 0.13.0 milestone Dec 25, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 26, 2024

The problem is that it is difficult to determine whether a given task is overridden or not at instantiation time.

  1. We cannot use any compile-time reflection at definition site, because it does not contain enough information (since any definition may or may not be overridden later)

  2. Java runtime reflection does not make it easy to distinguish real definitions from synthetic forwarders

  3. We want to avoid Scala runtime reflection due to its performance penalty

  4. We could use compile-time reflection on the final object that gets instantiated, but in general objects do not have any hook that allows us to trigger compile-time reflection to materialize this information

  5. Discover[T] can generate this information. But Discover is pretty frozen until Mill 0.13.0

Seems like we may need to wait for 0.13.0, and once we have the change to break bincompat we can update Discover to contain this information

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