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

[SYNPY-1575] Introduce EntityView model #1181

Merged
merged 98 commits into from
Apr 1, 2025

Conversation

BryanFauble
Copy link
Contributor

@BryanFauble BryanFauble commented Mar 11, 2025

Problem:

  1. The FileView aka EntityView was not created in an OOP fashion.
  2. There are a number of small bugs that I found while implementing this feature related to the work done in [SYNPY-1571] Adds Dataset Model & Introduces Composition Model for Table/View-like Classes #1175 and more generally the Table refactor: [SYNPY-1551] Tables refactor #1151

Solution:

  1. Creating a EntityView model based on this Synapse object: https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/table/EntityView.html
  2. Support the functionality for Views to be able to interact with the EntityViews
  3. Creating a tutorial and doc pages for the EntityView
  4. Patching an issue with re-ordering of Columns that I had found in testing. Essentially the problem is what when an existing View was stored the default columns were being ordered to the front of the table. To patch this issue I moved where default columns are being added to the .columns attribute to be after we get the existing object state from Synapse.
  5. Moving the async job to use the TQDM progress bar to prevent it from being printed out the console.
  6. Creating method wrappers in the EntityView class to allow for specific examples for that class type to be shown.

Testing:

  1. I wrote a number of integration tests to verify the async and non-async functionality.
  2. I wrote a tutorial that showcases how to use this EntityView model
  3. I ran through a significant amount of testing during development to verify functionality to creating, updating, adding, removing, data and columns

@BryanFauble BryanFauble changed the title [SYNPY-1575] Introduce FileView model [SYNPY-1575] Introduce EntityView model Mar 20, 2025
@BryanFauble BryanFauble changed the base branch from synpy-1571-dataset-model to develop March 25, 2025 15:41
@BryanFauble
Copy link
Contributor Author

@BWMac This could use a final review when you get a chance this week. Thanks in advance!

@@ -86,7 +86,6 @@ async def testCustomConfigFile(schedule_for_cleanup):
)


@pytest.mark.flaky(reruns=3, only_rerun=["SynapseHTTPError"])
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, how have these been resolved?

Copy link
Contributor Author

@BryanFauble BryanFauble Apr 1, 2025

Choose a reason for hiding this comment

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

It has not been explicitly fixed, but a global retry mechanism was put in place after this was written.

async def test_create_entityview_with_invalid_column(
self, project_model: Project
) -> None:
# GIVEN a entityview with an invalid column
Copy link
Member

Choose a reason for hiding this comment

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

I may have missed this, but what happens when an entity view is created with the default columns specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are specifying a column with the same name a warning is emitted and it is overwritten with the default column:

if self.include_default_columns:
view_type_mask = None
if self.view_type_mask:
if isinstance(self.view_type_mask, ViewTypeMask):
view_type_mask = self.view_type_mask.value
else:
view_type_mask = self.view_type_mask
default_columns = await get_default_columns(
view_entity_type=(
self.view_entity_type if self.view_entity_type else None
),
view_type_mask=view_type_mask,
synapse_client=synapse_client,
)
for default_column in default_columns:
if (
default_column.name in self.columns
and default_column != self.columns[default_column.name]
):
client.logger.warning(
f"Column '{default_column.name}' already exists in dataset. "
"Overwriting with default column."
)
self.columns[default_column.name] = default_column

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 LGTM! Awesome to see this work, I'll defer to brad for final review once he's back from vacay.

Copy link
Contributor

@BWMac BWMac left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@BryanFauble BryanFauble merged commit 3b5bc03 into develop Apr 1, 2025
28 checks passed
@BryanFauble BryanFauble deleted the synpy-1575-fileview-updates branch April 1, 2025 16:58
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