Remove AllFilter and NullTransformer and improve breadth/detail of logging#146
Merged
Remove AllFilter and NullTransformer and improve breadth/detail of logging#146
AllFilter and NullTransformer and improve breadth/detail of logging#146Conversation
Previously, the filter defaulted to an AllFilter and the transformer to a NullTransformer. This was nice from the perspective of code cleanliness because there were no branches for "is there a filter" or "is there a transformer." However, this did technically mean that everything was transformed (the transformer was an identity function). So when addressing #123 in a prior commit, this meant that using the default config for TaPS, every argument was getting "transformed" by the NullTransformer, resulting in tons of useless log lines. This commit changes the default filter and transformer to be the literal value None, rather than no-op implementations. The corresponding arguments to Engine, EngineConfig, and TaskTransformer were updated to support None values. Fortunately, the TaskTransformer wrapper means that all logic changes were isolated there (i.e., the branches for if a filter/transformer were configured). A tricky side-effect of this change is that _make_config_cls expects all engine plugins to have a value so this required some changes to support the None value path. Got that all fixed up and along the way improved some of the outputs in the CLI help.
gpauloski
added
breaking
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Note: This is a breaking change mostly because old configs that use the "all" and "null" filter and "null" transformer will fail to parse. The default behavior of TaPS is still the same.
This PR was originally about improving logging, which it does. As a part of improving logging was adding TRACE logging when an object is transformed---a helpful feature for debugging filter configurations. However, TaPS would default to an identity transformer so the logging wasn't very helpful when everything is technically transformed. I changed the transformer/filter system to support
Noneoptions, which results in the aforementioned breaking config change.Changes:
TRACElogging levelrepr()implementations to many classesAllFilterandNullTransformerNullFiltertoNeverFilterTaskInfo.function_nameTaskInfo.nameto reflect custom task names introduced in Add the@task()decorator so task functions can be serialized by reference #143Engine/EngineConfignow default toNoneTaskTransformercan be used as a context manager (good for testing)Fixes
DEBUGlogging for data transformation #123Type of Change
Testing
Ran lots of benchmarks and updated unit tests where needed.
Pull Request Checklist
Please confirm the PR meets the following requirements.
pre-commit(e.g., ruff, mypy, etc.).