Repository navigation
Restore get_pmax_performance, get_pmax_assets and get_detailed_asset_performance - #125
Open
illia-sapryga wants to merge 1 commit into
Open
illia-sapryga wants to merge 1 commit into
illia-sapryga wants to merge 1 commit into
Conversation
draft_asset_group_assets replaced pmax.py and dropped get_pmax_performance, get_asset_performance and get_detailed_asset_performance. The wrappers in server.py import them when called, so all three tools failed with an import error at call time while the server started and tests passed. Add them back unchanged, plus a test that every tool wrapper's lazy import resolves.
This branch has not been deployed
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
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.
get_pmax_performance,get_pmax_assetsandget_detailed_asset_performanceall fail when called:d650c24 (
draft_asset_group_assets) rewroteads/pmax.pyand the three read functions went with it. The wrappers inserver.pyimport them inside the function body, so the server starts, the tools are listed, and only a call fails. No test calls them, which is why CI stayed green. It is in every release since that commit, including 0.37.0.This adds the three functions back exactly as they were before d650c24, under a "Read tools" section, along with the two imports they need.
tests/test_pmax_read.pycovers each of the three functions with a stubbed query layer. It also walksserver.pyfor everyfrom adloop... import xinside a function and checks that it resolves, so a function going missing from any tool module fails CI instead of failing at call time. That check passes for all the other tools today, so these three were the only ones affected.What it does not do: I did not change what the functions return.
get_pmax_performancestill lists removed campaigns and their asset groups, so its ad-strength insight can flag a group in a removed campaign. That was already the behaviour, and it could be a separate change if you want it.Tested with the full suite, and I ran all three functions read-only against a live Performance Max account: campaigns, asset groups and assets came back as expected.
get_detailed_asset_performancereturned no error but zero combinations on that account, so its row parsing is covered by neither the live run nor the test.