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

Fix Scala using add_dependencies_on_all_siblings=True #14382

Closed
stuhood opened this issue Feb 6, 2022 · 3 comments · Fixed by #15841
Closed

Fix Scala using add_dependencies_on_all_siblings=True #14382

stuhood opened this issue Feb 6, 2022 · 3 comments · Fixed by #15841
Labels
backend: JVM JVM backend-related issues bug

Comments

@stuhood
Copy link
Member

stuhood commented Feb 6, 2022

After dependency inference was improved for Scala, add_dependencies_on_all_siblings was not removed:

add_dependencies_on_all_siblings=True,
This means that we are overly coarsening Scala (all BUILD targets end up compiled together).

We should fix this (or drive it via an option), but it will definitely impact compilation success rates, and might also impact performance. We should test out the impact in our testbed repositories.

@stuhood stuhood added the bug label Feb 6, 2022
@tdyas tdyas added the backend: JVM JVM backend-related issues label Feb 16, 2022
@somdoron
Copy link
Contributor

I've experimented with removing this flag on our codebase, which required some more improvements to scala parser, after those improvements (PR is coming), the number of coarsened targets increased from 80 to around 800. It also allowed us to reduce process_per_child_memory_usage and increase the parallelism.

The best benefit for us would be less cache invalidation of test results.

I would like to contribute a PR to address this. Should I remove the flag or allow it to be configurable?

@stuhood
Copy link
Member Author

stuhood commented Jun 13, 2022

I would like to contribute a PR to address this. Should I remove the flag or allow it to be configurable?

Thanks a ton! It will need to be configurable for sure, in order to allow folks to migrate from the existing default. But I expect that we can begin deprecating the existing value immediately.

@Eric-Arellano
Copy link
Contributor

Awesome! For other languages, we set it dynamically based on if dependency inference is enabled:

@rule
def python_files_generator_settings(
_: PythonFilesGeneratorSettingsRequest,
python_infer: PythonInferSubsystem,
) -> TargetFilesGeneratorSettings:
return TargetFilesGeneratorSettings(add_dependencies_on_all_siblings=not python_infer.imports)

UnionRule(TargetFilesGeneratorSettingsRequest, PythonFilesGeneratorSettingsRequest),

We will want to preserve that. Without dependency inference, it is incredibly tedious to specify "sibling" dependencies and you have to do it via the overrides field.

So, I suspect we maybe also have a temporary option like [scala-infer].force_sibling_dependencies with a default to true, per our deprecation policy. We deprecate not setting the default, and then possibly deprecate the option entirely in the subsequent release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: JVM JVM backend-related issues bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants