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

Use canonical fully qualified class names for metrics #1680

Merged
merged 9 commits into from
Aug 8, 2023

Conversation

yifanmai
Copy link
Collaborator

The current implementation of create_object() is obsolete and incorrect; see this answer for the correct version.

This also means we don't need to keep adding names to the __init__.py file.

Addresses #1327

@yifanmai yifanmai requested review from percyliang and teetone June 21, 2023 00:24
@yifanmai
Copy link
Collaborator Author

Note that the current JSON data release does not use the canonical fully-qualified class names; we could use a script to rewrite the JSON to the correct names.

@yifanmai yifanmai force-pushed the yifanmai/1327-split-metrics branch from e2b5ec9 to b98ac8b Compare June 21, 2023 00:42
@@ -1,98 +0,0 @@
# Add any classes that need to be loaded dynamically via `create_object`.
Copy link
Member

Choose a reason for hiding this comment

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

This is great :)

@teetone teetone self-requested a review June 22, 2023 02:17
@yifanmai yifanmai force-pushed the yifanmai/1327-split-metrics branch from 6347118 to 89ba7de Compare June 26, 2023 21:28
@yifanmai
Copy link
Collaborator Author

mypy is failing due to an unrelated issue -install-types failed (no mypy cache directory); see python/mypy#10600

@yifanmai yifanmai force-pushed the yifanmai/1327-split-metrics branch from 89ba7de to ff3c770 Compare August 7, 2023 23:43
Comment on lines +23 to +25
class_name = components[-1]
module_name = ".".join(components[:-1])
cls = getattr(importlib.import_module(module_name), class_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern shows up three times...could we factor it out or put a comment that we need to sync?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added todo.

@yifanmai yifanmai merged commit 6976ca9 into main Aug 8, 2023
@yifanmai yifanmai deleted the yifanmai/1327-split-metrics branch August 8, 2023 06:14
@yifanmai yifanmai mentioned this pull request Aug 10, 2023
danielz02 pushed a commit to danielz02/helm that referenced this pull request Sep 7, 2023
danielz02 pushed a commit to danielz02/helm that referenced this pull request Sep 7, 2023
danielz02 pushed a commit to danielz02/helm that referenced this pull request Sep 7, 2023
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.

3 participants