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

feat: dont use group name as primary key #1507

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

fengelniederhammer
Copy link
Contributor

@fengelniederhammer fengelniederhammer commented Mar 28, 2024

resolves #1157

Please review and merge #1500 and #1505 first.

preview URL: https://1157-dont-use-group-name.loculus.org/

Summary

URLs use the group id now in the backend and on the website (e.g. /group/123).

Screenshot

grafik

PR Checklist

  • [ ] All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

@fengelniederhammer fengelniederhammer added the preview Triggers a deployment to argocd label Mar 28, 2024
@fengelniederhammer fengelniederhammer force-pushed the 1157-dont-use-group-name-as-primary-key_2 branch from 652aa35 to e0a1e1f Compare March 28, 2024 09:33
@chaoran-chen chaoran-chen changed the base branch from main to 1157-dont-use-group-name-as-primary-key March 28, 2024 11:07
@chaoran-chen chaoran-chen changed the base branch from 1157-dont-use-group-name-as-primary-key to main March 28, 2024 11:07
@fengelniederhammer fengelniederhammer force-pushed the 1157-dont-use-group-name-as-primary-key_2 branch from e0a1e1f to 13ca5c9 Compare March 28, 2024 11:41
Copy link
Contributor

@JonasKellerer JonasKellerer left a comment

Choose a reason for hiding this comment

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

LGTM

@fengelniederhammer fengelniederhammer force-pushed the 1157-dont-use-group-name-as-primary-key_2 branch from 13ca5c9 to 2933576 Compare March 28, 2024 13:02
Copy link
Member

@chaoran-chen chaoran-chen left a comment

Choose a reason for hiding this comment

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

https://1157-dont-use-group-name.loculus.org/dummy-organism/my_sequences/insdc_ingest_group?

I am getting an error message here:

image

I didn't check whether this was already the case in the past or introduced by this PR, but now I see the group twice in the dropdown menu (on the same page as above):

image

@fengelniederhammer
Copy link
Contributor Author

Thanks, good catch. group is indeed not a field anymore, it should be groupName or groupId.

Seing the same group twice might be a result of the fact that the group actually exists twice? How is this group created?

@corneliusroemer
Copy link
Contributor

Must be a bug somewhere, a new group seems to be created whenever one logs in:

image

The group is created by the ingest pipeline, the ingest pipeline checks whether a certain group exists, if it doesn't, it recreates.

So the PR requires some adjustment of the ingest code!

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

Ingest group creation logic needs to be adjusted to prevent repeated creation of the same insdc_ingest_group multiple times

Relevant code is here:
https://github.com/loculus-project/loculus/blob/2933576b5eeebdf431325a2863f44333addecee8/ingest/scripts/submit_to_loculus.py#L49-L79

@corneliusroemer
Copy link
Contributor

Should we hide that groupid? It doesn't contain any useful information?
image

@fengelniederhammer
Copy link
Contributor Author

I tried fixed the website (I found a couple more forgotten adaption).

@corneliusroemer I tried to adapt the script. Can you please check whether that might be correct? My Python isn't that good and I don't know how to test whether it works.

@fengelniederhammer
Copy link
Contributor Author

Should we hide that groupid? It doesn't contain any useful information?

We could make it less prominent, but I added it because it's the relevant id now.

@chaoran-chen
Copy link
Member

https://1157-dont-use-group-name.loculus.org/ebola-zaire/my_sequences/1? is empty, which is a sign that the ingest script is not working?

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

I've made it work with the ingest pipeline now, thanks for getting started with it @fengelniederhammer - there was a small bug but it's my bad as I have not documented how to test the ingest pipeline at all, other than in CI.

@chaoran-chen it works now:
image

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

Group id doesn't seem to make it to lapis:
image
https://1157-dont-use-group-name.loculus.org/seq/LOC_0001HFK.1

Chaoran's point also is still valid, that my sequences doesn't show anything despite sequences being there:
https://1157-dont-use-group-name.loculus.org/ebola-zaire/my_sequences/1?

@fengelniederhammer fengelniederhammer force-pushed the 1157-dont-use-group-name-as-primary-key_2 branch from 6807e86 to 2fe000f Compare April 2, 2024 09:30
@fengelniederhammer
Copy link
Contributor Author

I rebased on the latest main.

The group id does make it to LAPIS: https://1157-dont-use-group-name.loculus.org/seq/LOC_000007L.1
grafik

Also the ingest seems to work: https://1157-dont-use-group-name.loculus.org/ebola-zaire/search?
grafik
(Thanks @corneliusroemer for fixing!)

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

Looks great! All issues appear fixed

@fengelniederhammer fengelniederhammer force-pushed the 1157-dont-use-group-name-as-primary-key_2 branch from 0a93ecd to f33138f Compare April 2, 2024 10:46
@corneliusroemer corneliusroemer changed the title dont use group name as primary key feat: dont use group name as primary key Apr 2, 2024
@corneliusroemer
Copy link
Contributor

corneliusroemer commented Apr 2, 2024

I've improved search ui config: #1541, can be merged into this PR
image

@fengelniederhammer fengelniederhammer merged commit 3bc1966 into main Apr 2, 2024
13 checks passed
@fengelniederhammer fengelniederhammer deleted the 1157-dont-use-group-name-as-primary-key_2 branch April 2, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't use group name as primary key
4 participants