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

Fix an issue where the asset view was not being set correctly for operations involving manifest validation #1312

Merged
merged 10 commits into from
Oct 23, 2023

Conversation

GiaJordan
Copy link
Contributor

@GiaJordan GiaJordan commented Oct 11, 2023

Add asset_view to /model/validate and project_scope to /model/validate and model/submit so that cross manifest validation operations locate the correct resources.

Depends on #1311 #1314

If you run any of the changed unit tests please delete the manifests that were uploaded to datasets syn52656106 or syn52656104 immediately to avoid causing other branches tests to fail. (Required until #1314 is merged)

@GiaJordan GiaJordan changed the title Develop api validation asset view fds 810 Fix an issue where the asset view was not being set correctly for operations involving manifest validation Oct 13, 2023
@GiaJordan GiaJordan requested review from linglp and mialy-defelice and removed request for linglp October 13, 2023 17:00
Base automatically changed from develop-validation-access-token-FDS-904 to develop October 16, 2023 16:49
@Sage-Bionetworks Sage-Bionetworks deleted a comment from dpulls bot Oct 16, 2023
@GiaJordan GiaJordan mentioned this pull request Oct 17, 2023
@linglp
Copy link
Contributor

linglp commented Oct 18, 2023

Did some tests manually below through API:

  1. Validate an invalid manifest using the /model/validate endpoint with project_scope and asset_view parameter. Ensure that errors could be returned as expected.
  2. Validate a valid manifest using the /model/validate using asset view: syn23643253 and project scope: syn23643250. Everything seems to be working too.
  3. I also created a brand new project with a brand new dataset folder and asset view to test it out. (Asset view: syn52737292 and project scope: syn52737126) and by uploading a valid test manifest, the output that I got is:
{
  "errors": [],
  "warnings": [
    [
      [],
      "Check Match at Least",
      "Value(s) [] from row(s) [] of the attribute Check Match at Least in the source manifest are missing.",
      []
    ]
  ]
}

I don't think this is expected because it doesn't show the values and rows that don't match. Also, this warning shouldn't be shown because in my test folder: https://www.synapse.org/#!Synapse:syn52737592, I have the same manifest.

Another point to note:
Currently, if there are no datasest ids or manifest ids found within a given project, the code would continue without giving any warning. For debugging, I think it will bee helpful to note that if there are no dataset ids found or no manifest ids found so that users know that cross manifest validation didn't really get triggered.

Another observation is that if we have three columns that have validation rules that could trigger cross manifest, it seems like we are calling cross_validation three times? And each time that we call cross_validation function , we are downloading manifests within a given project. If this is correct, maybe this part could be optimized in the future..

@GiaJordan
Copy link
Contributor Author

@linglp

  1. I also created a brand new project with a brand new dataset folder and asset view to test it out. (Asset view: syn52737292 and project scope: syn52737126) and by uploading a valid test manifest, the output that I got is:
{
  "errors": [],
  "warnings": [
    [
      [],
      "Check Match at Least",
      "Value(s) [] from row(s) [] of the attribute Check Match at Least in the source manifest are missing.",
      []
    ]
  ]
}

I don't think this is expected because it doesn't show the values and rows that don't match. Also, this warning shouldn't be shown because in my test folder: https://www.synapse.org/#!Synapse:syn52737592, I have the same manifest.
Another point to note: Currently, if there are no datasest ids or manifest ids found within a given project, the code would continue without giving any warning. For debugging, I think it will bee helpful to note that if there are no dataset ids found or no manifest ids found so that users know that cross manifest validation didn't really get triggered.

Discussed in CR, to be addressed in FDS-1236

Another observation is that if we have three columns that have validation rules that could trigger cross manifest, it seems like we are calling cross_validation three times? And each time that we call cross_validation function , we are downloading manifests within a given project. If this is correct, maybe this part could be optimized in the future..

Discussed in CR, to be addressed in FDS-1237

cc: @AmyHeiser

Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

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

Thanks @GiaJordan ! i also confirmed that the submission is working fine.

@GiaJordan
Copy link
Contributor Author

@linglp thank you for reviewing!

@dpulls
Copy link

dpulls bot commented Oct 20, 2023

🎉 All dependencies have been resolved !

@GiaJordan GiaJordan merged commit f098383 into develop Oct 23, 2023
@GiaJordan GiaJordan deleted the develop-api-validation-asset-view-FDS-810 branch October 23, 2023 20:52
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