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.

Sign up for GitHub

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

Jump to bottom

Fix JVM fingerprinting by adding tags #8175

Merged
merged 3 commits into from
Aug 16, 2019
Merged

Fix JVM fingerprinting by adding tags #8175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30470b2
Fix JVM fingerprinting by adding tags and workflow info
wisechengyi Aug 15, 2019
eed65cc
Only add tags to fingerprint
wisechengyi Aug 16, 2019
e9b4c3a
use stable_json_sha1
wisechengyi Aug 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/python/pants/backend/jvm/subsystems/dependency_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ def all_dependencies(self, target):
for dep in target.closure(bfs=True, **self.target_closure_kwargs):
yield dep

def create_fingerprint_strategy(self, classpath_products):
return ResolvedJarAwareFingerprintStrategy(classpath_products, self)
def create_fingerprint_strategy(self, classpath_products, additional_confs=None):
return ResolvedJarAwareFingerprintStrategy(classpath_products, self, additional_confs)

def defaulted_property(self, target, option_name):
"""Computes a language property setting for the given JvmTarget.
Expand Down Expand Up @@ -74,10 +74,11 @@ def dependencies_respecting_strict_deps(self, target):
class ResolvedJarAwareFingerprintStrategy(FingerprintStrategy):
"""Task fingerprint strategy that also includes the resolved coordinates of dependent jars."""

def __init__(self, classpath_products, dep_context):
def __init__(self, classpath_products, dep_context, additional_confs):
super().__init__()
self._classpath_products = classpath_products
self._dep_context = dep_context
self._additional_confs = additional_confs

def compute_fingerprint(self, target):
if isinstance(target, Resources):
Expand All @@ -86,6 +87,8 @@ def compute_fingerprint(self, target):

hasher = hashlib.sha1()
hasher.update(target.payload.fingerprint().encode())
# Adding tags into cache key because it may decide which workflow applies to the target.
hasher.update(','.join(sorted(target.tags)).encode())
wisechengyi marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(target, JarLibrary):
# NB: Collects only the jars for the current jar_library, and hashes them to ensure that both
# the resolved coordinates, and the requested coordinates are used. This ensures that if a
Expand All @@ -96,6 +99,10 @@ def compute_fingerprint(self, target):
[target])
for _, entry in classpath_entries:
hasher.update(str(entry.coordinate).encode())

for conf in self._additional_confs:
hasher.update(conf.encode())

return hasher.hexdigest()

def direct(self, target):
Expand Down
5 changes: 4 additions & 1 deletion src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,9 @@ def create_compile_context(self, target, target_workdir):
'post_compile_merge_dir'),
sources=self._compute_sources_for_target(target))

def additional_confs_for_fignerprinting(self):
wisechengyi marked this conversation as resolved.
Show resolved Hide resolved
return []

def execute(self):
requested_compiler = JvmPlatform.global_instance().get_options().compiler
if requested_compiler != self.compiler_name:
Expand Down Expand Up @@ -422,7 +425,7 @@ def execute(self):
classpath_product = self.create_runtime_classpath()

fingerprint_strategy = DependencyContext.global_instance().create_fingerprint_strategy(
classpath_product)
classpath_product, self.additional_confs_for_fignerprinting())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
classpath_product, self.additional_confs_for_fignerprinting())
classpath_product, self.additional_confs_for_fingerprinting())

# Note, JVM targets are validated (`vts.update()`) as they succeed. As a result,
# we begin writing artifacts out to the cache immediately instead of waiting for
# all targets to finish.
Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ def mirrored_target_option_actions(self):
def implementation_version(cls):
return super().implementation_version() + [('RscCompile', 173)]

def additional_confs_for_fignerprinting(self):
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Hmm... this is supposed to be accounted for by

pants/src/python/pants/task/task.py

Lines 190 to 192 in aa47d93

@memoized_property
def _build_invalidator(self):
return BuildInvalidator.Factory.create(build_task=self.fingerprint)
... is there a missing Subsystem dependency here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't appear that workflow was added to the scala_library()'s payload, which is what I thought was used for fingerprinting:

pants/src/python/pants/backend/jvm/targets/scala_library.py

Lines 55 to 58 in aa47d93

payload = payload or Payload()
payload.add_fields({
'java_sources': PrimitiveField(self.assert_list(java_sources, key_arg='java_sources')),
})
.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or in JvmTarget:

pants/src/python/pants/backend/jvm/targets/jvm_target.py

Lines 93 to 108 in aa47d93

payload = payload or Payload()
excludes = ExcludesField(self.assert_list(excludes, expected_type=Exclude, key_arg='excludes'))
payload.add_fields({
'sources': self.create_sources_field(sources, address.spec_path, key_arg='sources'),
'provides': provides,
'excludes': excludes,
'platform': PrimitiveField(platform),
'strict_deps': PrimitiveField(strict_deps),
'exports': PrimitivesSetField(exports or []),
'compiler_option_sets': PrimitivesSetField(compiler_option_sets),
'zinc_file_manager': PrimitiveField(zinc_file_manager),
'javac_plugins': PrimitivesSetField(javac_plugins or []),
'javac_plugin_args': PrimitiveField(javac_plugin_args),
'scalac_plugins': PrimitivesSetField(scalac_plugins or []),
'scalac_plugin_args': PrimitiveField(scalac_plugin_args),
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuhood that makes sense, although in practice changing --workflow resulted the same fingerprint, so likely something is missing.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@wisechengyi : Perhaps we're not fingerprinting the "enum" type in a useful way...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried converting --workflow to a str type option, and target fingerprint isn't changed either, so likely it's just not taken into consideration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly false alarm, as I only printing the target key instead of the full key.

pants/src/python/pants/invalidation/build_invalidator.py

Lines 95 to 106 in 14f079f

def key_for_target(self, target, transitive=False, fingerprint_strategy=None):
hasher = self._base_hasher.copy()
key_suffix = hasher.hexdigest()[:12]
if transitive:
target_key = target.transitive_invalidation_hash(fingerprint_strategy)
else:
target_key = target.invalidation_hash(fingerprint_strategy)
if target_key is not None:
full_key = '{target_key}_{key_suffix}'.format(target_key=target_key, key_suffix=key_suffix)
return CacheKey(target.id, full_key)
else:
return None

return [self.get_options().workflow.value]

class JvmCompileWorkflowType(enum(['zinc-only', 'zinc-java', 'rsc-and-zinc'])):
"""Target classifications used to correctly schedule Zinc and Rsc jobs.

Expand Down