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 subclass of sync #8431

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Fix subclass of sync #8431

merged 2 commits into from
Nov 22, 2024

Conversation

matentzn
Copy link
Member

@matentzn matentzn commented Nov 21, 2024

Previously the syncing was wrong as the method was deleting all subclass of axioms, regardless of whether they were existential restrictions or subclass between names. It took a while to find this solution, but I think it should work:

  1. delete all subclass of axioms after
  2. backing up all subclass of axioms with existentials
  3. backing uo all subclass of axioms without existentials, and then removing evidence from all the mondo ingest primary sources from it.

I accidentally merged the synonym sync commit with the URL change but this will disappear since #8430 is merged.

Previously the syncing was wrong as the method was deleting _all_ subclass of axioms, regardless of whether they were existential restrictions or subclass between names. It took a while to find this solution, but I think it should work:

1. delete _all_ subclass of axioms after
2. backing up all subclass of axioms with existentials
3. backing uo all subclass of axioms without existentials, and then removing evidence from all the mondo ingest primary sources from it.
@twhetzel
Copy link
Collaborator

twhetzel commented Nov 21, 2024

@matentzn does this mean that #8422 should not be merged? The results of running the Synonym Sync pipeline before this change?

cc: @sabrinatoro

@matentzn
Copy link
Member Author

matentzn commented Nov 21, 2024

@twhetzel #8422 is such a tiny PR, I would just finalise and merge rather than doing the thinking again!

Copy link
Collaborator

@twhetzel twhetzel left a comment

Choose a reason for hiding this comment

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

Works for me based on what we discussed. However, I did not run this goal locally to test.

@twhetzel
Copy link
Collaborator

@matentzn - do you still want this ticket merged despite the data issues with the Synonym Sync posted here: #8432? In this PR it's just code, but I also do not know why you didn't merge this after it was approved so that's why I am asking.

@matentzn
Copy link
Member Author

I am on the fence - the diff was so large on the data load #8432 that we cannot with all confidence say that, once the bug is fixed, no other issue emerges.. I am fine merging it, but I am also fine waiting, as we are not going to run subclass sync again until the bug is fixed

@twhetzel
Copy link
Collaborator

Ok, sounds like we should merge it then to clear out PRs and make any future updates as needed once the bug in generating the subclass sync data is sorted since as you say the subclass sync will not be run otherwise.

@twhetzel twhetzel merged commit 7c58bb4 into master Nov 22, 2024
1 check passed
@twhetzel twhetzel deleted the fix-subclass-of-sync branch November 22, 2024 17:56
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