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

PTV-1810 fix obj input 3 #3322

Merged
merged 14 commits into from
Aug 25, 2023
Merged

PTV-1810 fix obj input 3 #3322

merged 14 commits into from
Aug 25, 2023

Conversation

briehl
Copy link
Member

@briehl briehl commented Jul 31, 2023

Description of PR purpose/changes

Actually go through the code to update how object inputs get matched here.

There's a few minor details and code styling yet to do on app_util.transform_param_value, but this is ready for GHA testing / review.

The goal, as mentioned elsewhere (#3320) is to update the transform_param_value function to effectively take any form of object input and transform it appropriately. Previously, it was implicitly assumed that input values were always object names, and transforms were only done when target_type_transform was present. Now, however, inputs can be of any format, and combinations of target_type_transform are used to infer what's required.

  • target_type_transform not present = object name string output
  • target_type_transform = "ref" or "unresolved-ref" = ws_name/obj_name format
  • target_type_transform = "resolved-ref" or "upa" (not officially supported, but should be) = upa / path format

TODO:

  • make sure ref paths work as expected, when expected
  • clean up some minor code duplications
  • double-check comments
  • add release notes

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/PTV-1810

  • Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

  • Details for how to test the PR:
  • Tests pass locally and in GitHub Actions
  • Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

  • My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • (JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook
  • (Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook
  • Any dependent changes have been merged and published in downstream modules

Updating Version and Release Notes (if applicable)

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #3322 (d869e99) into develop (4580595) will increase coverage by 23.63%.
Report is 17 commits behind head on develop.
The diff coverage is 89.09%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #3322       +/-   ##
============================================
+ Coverage     5.62%   29.25%   +23.63%     
============================================
  Files          496      496               
  Lines        50486    50512       +26     
============================================
+ Hits          2841    14779    +11938     
+ Misses       47645    35733    -11912     
Files Changed Coverage Δ
src/biokbase/narrative/app_util.py 82.93% <89.09%> (-1.29%) ⬇️

... and 326 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70d3b26...d869e99. Read the comment docs.

@briehl briehl marked this pull request as draft August 2, 2023 14:39
@briehl briehl marked this pull request as ready for review August 7, 2023 23:49
@@ -7,7 +7,7 @@ This is built on the Jupyter Notebook v6.4.12 and IPython 8.5.0 (more notes will
## Unreleased
- PTV-1875 - fix public data paging issue by removing paging from workspace data sources
- UIP-28 - update Google Analytics tags to GA4 properties
- PTV-1810 - address object name display issues in the View Configure tab of app cells.
- PTV-1810 - address object name display issues in the View Configure tab of app cells. This now saves all app inputs as UPAs in the cell. It also includes an update to input transforms to properly convert from UPAs <-> names or references as appropriate before starting the app.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉


if is_path:
return ";".join(obj_info["paths"][0])
if transform_type == "ref" or transform_type == "unresolved-ref":
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if it'd be marginally more readable as

if transform_type in ["ref", "unresolved-ref"]:

Probably not enough to make it worth changing.

ialarmedalien
ialarmedalien previously approved these changes Aug 11, 2023
@ialarmedalien ialarmedalien dismissed their stale review August 11, 2023 15:34

PR still in progress

Base automatically changed from PTV-1810-fix-obj-input-2 to develop August 14, 2023 17:57
@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -292,7 +292,7 @@
"widget_info": {
"name": "no-display",
"params": {
"obj_ref": "18836/5/1",
"obj_ref": "62425/2/12",
Copy link
Member Author

@briehl briehl Aug 24, 2023

Choose a reason for hiding this comment

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

These were put in place (I think) because the mocker was switching them around to the one true mock UPA.

Switching them back to the correct UPA seems to have fixed the tests.

@@ -401,7 +401,7 @@
"widget_info": {
"name": "no-display",
"params": {
"obj_ref": "18836/5/1",
"obj_ref": "62425/2/12",
Copy link
Member Author

Choose a reason for hiding this comment

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

Same thing as above. The mocked upas were bleeding over into the returned test data.

num_objects = len(params.get("objects", [0]))
return {"infos": infos * num_objects, "paths": paths * num_objects}
paths = []
Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusts the mock so if a test calls get_object_info3 with an N-step reference path, like:

{"objects": [{"ref": "ws1/obj1;ws2/obj2"}]}

It returns:

{
    "infos": standard mocked object info stuff,
    "paths": [["1/1/1", "2/2/2", ... "N-1/N-1/N-1", "18836/5/1"]] (the mocked upa)
}

Really, this should get factored into a pytest fixture and take expected ins and outs, but that's a bigger project.

@briehl briehl merged commit b2688da into develop Aug 25, 2023
7 of 9 checks passed
@briehl briehl deleted the PTV-1810-fix-obj-input-3 branch August 25, 2023 17:48
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.

2 participants