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

Dev dump #158

Merged
merged 15 commits into from
Nov 26, 2024
Merged

Dev dump #158

merged 15 commits into from
Nov 26, 2024

Conversation

bamader
Copy link
Collaborator

@bamader bamader commented Nov 13, 2024

Mount a pg_dump extract to docker dev

Summary

This PR does a lot of things:

  • Creates a constants file for holding SQL strings and structs used for database insertion
  • Extracts all of the default query / hard coded information that we had in the flyway migrations into individual asset files for separate loading
  • Docker mounts the assets directory into the container
  • Refactors some of the database-service code to use the new SQL constants
  • Adds a new abstracted DB Struct insertion method so that we can insert via file reads
  • Completes the db creation backend script that automatically adds and indexes everything into a ready DB
  • Updates parsing of the eRSD so that we extract conditions and related value sets
  • Uploads a .sql dump of the PG database after all eRSD data has been parsed and all DIBBs custom cases have been loaded
  • Mounts the dev dump for use in both dev environments and e2e testing
  • Removes 5 now obsolete flyway migrations
  • Actually recovers one additional chlamydia lab valueset due to an incorrect identifier ordering in seeding data (was listed as version_code when it should have been code_version)

Related Issue

Fixes #122

Additional Information

Added a readme with the command needed to create the dump file in case we ever want to update the dev dump. Code tested by nuking all images, docker system pruneing, then just running npm run dev and verifying values were in the DB via DBeaver.

Copy link
Collaborator

@m-goggins m-goggins left a comment

Choose a reason for hiding this comment

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

Looks good! I believe we need to mount the same volume to the db docker-compose-e2e so that the data is available for end to end tests.

@bamader
Copy link
Collaborator Author

bamader commented Nov 14, 2024

Actually that reminds me that we need to also call the valueset insertion functions for the DIBBs hardcoded values / JSON dump file that you wrote at /src/app/assets/DIBBS_Custom_ValueSets.json in the create DB functions overall. This dump doesn't include those so our e2e's will totally fail. I'll update the code and the dump mount.

Copy link
Collaborator

@robertandremitchell robertandremitchell left a comment

Choose a reason for hiding this comment

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

makes sense to me. I wonder if we actually want to commit the vs_dump.sql to github or just have it locally? or do we need to upload it for tests to pass successfully?

@bamader
Copy link
Collaborator Author

bamader commented Nov 14, 2024

@robertandremitchell I believe we'll need to upload it for tests to pass once we take the migrations out. I'll have to modify the github workflow to take advantage of it. I think it's also probably good to commit it in case anyone else needs to pull things down and be able to iterate quickly, e.g. a designer who wants to test out a new feature's appearance.

Copy link
Collaborator

@robertandremitchell robertandremitchell left a comment

Choose a reason for hiding this comment

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

running into some issues getting this to run either in dev mode or docker. I ran into some of these docker issues on Marcelle's old PRs, so not sure how much this is new, but attached is what I am currently running into. It is working to...some degree for me in dev, while it's breaking entirely in docker. below is the screenshot of what happens when I click the Create Query button.

Screenshot 2024-11-22 at 10 07 02 AM

error_message.txt

query-connector/VS_DUMP_INFO.md Outdated Show resolved Hide resolved
@bamader
Copy link
Collaborator Author

bamader commented Nov 22, 2024

@robertandremitchell Actually your error log helped me find a problem! In dev mode, we should never need to be accessing those seeding scripts because it loads everything from the DB mount--i.e. we only seed from a docker run where we're trying to do the data dump, not once we're already in dev mode. I've moved those inserts into the if check for whether there's already data in the DB, so see if that helps!

@robertandremitchell
Copy link
Collaborator

@robertandremitchell Actually your error log helped me find a problem! In dev mode, we should never need to be accessing those seeding scripts because it loads everything from the DB mount--i.e. we only seed from a docker run where we're trying to do the data dump, not once we're already in dev mode. I've moved those inserts into the if check for whether there's already data in the DB, so see if that helps!

thanks! that seems to have fixed dev mode for me. still trying to figure out what is happening with my docker error when I click the button. This is the same as I was getting on Marcelle's prior PR so idk if it's something specific to my docker. I'll try updating, pruning, then building fresh 🤞🏽

query-connector-1  | TypeError: Cannot read properties of undefined (reading '0')
query-connector-1  |     at /app/.next/server/chunks/652.js:122:2119
query-connector-1  |     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
query-connector-1  |     at async C (/app/.next/server/chunks/652.js:122:2089)
query-connector-1  |     at async /app/.next/server/app/queryBuilding/page.js:1:5766
query-connector-1  |     at async Promise.all (index 50)
query-connector-1  |     at async c (/app/.next/server/app/queryBuilding/page.js:1:5722)
query-connector-1  |     at async d (/app/.next/server/app/queryBuilding/page.js:1:7354)
query-connector-1  |     at async /app/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:16:418
query-connector-1  |     at async rE (/app/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:15:7978)
query-connector-1  |     at async r7 (/app/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:18:1144)

@robertandremitchell
Copy link
Collaborator

@bamader , with some help from Marcelle and a fresh Docker install, I seem to have made some progress. Based on the log (attached), it broadly seems to have worked. I have a count of 226 conditions, 33810 conditions, and 1390 valuesets, which broadly seems right. However, category is NULL for all conditions, which is leading to a weird display:

Screenshot 2024-11-25 at 9 30 57 AM

I'm not sure if this is a sequencing issue with the migrations and the creation of the eRSD data, or something else. At some point, I had gotten this to render correctly on npm run dev; however, now I'm getting the same issue with null category.

log.txt

@bamader
Copy link
Collaborator Author

bamader commented Nov 25, 2024

@robertandremitchell Yeah it looks like a sequencing problem. Migration 7 is the category data which currently in main relies on a staging table that's a bit harder to use than standard pulldowns. I think I should be able to just make this an additional asset during seeding though, so let me give that a try. Good find!

@m-goggins
Copy link
Collaborator

I ran into no errors and things started up super quickly when I ran npm run dev-win but I've got slightly different numbers from Rob in the DB 😬 228 conditions, 1406 valuesets, and 34,034 concepts

@robertandremitchell
Copy link
Collaborator

I ran into no errors and things started up super quickly when I ran npm run dev-win but I've got slightly different numbers from Rob in the DB 😬 228 conditions, 1406 valuesets, and 34,034 concepts

I've run it a few different times and sometimes get Newborn Screening and Cancer back, which are the two missings ones and I think responsible for the downwind deltas.

I...could not tell you why I sometimes get them 😓 , I've tried a few times to try to replicate.

@bamader
Copy link
Collaborator Author

bamader commented Nov 25, 2024

@robertandremitchell Okay category data should be tracked now (and as an added plus, we get to delete off the last non-setup migration!). Also, from a fully fresh load on both docker compose and npm run dev, I see the exact counts of valuesets, concepts, and conditions that Marcelle does (which I also know from hand counting, at least on the condition side, is actually correct 😅 ... yes I had to do that at one point...)

Copy link
Collaborator

@robertandremitchell robertandremitchell 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! worked for me, accept these two conditions had no categories, for me:

Invasive Group B beta-hemolytic streptococcal disease

Invasive Group A beta-hemolytic streptococcal disease

@bamader
Copy link
Collaborator Author

bamader commented Nov 25, 2024

@robertandremitchell Ah good find, the parsing script I used must've clipped a code or something. I'll add that back in!

@bamader
Copy link
Collaborator Author

bamader commented Nov 25, 2024

Okay missing condition categories should now be fixed. @robertandremitchell Want to give you a final checking chance before I merge anything so let me know when you're satisfied!

Copy link
Collaborator

@robertandremitchell robertandremitchell left a comment

Choose a reason for hiding this comment

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

looks good! thanks to you both @m-goggins and @bamader for helping me debug!

@bamader bamader merged commit c7d86ad into main Nov 26, 2024
5 checks passed
@bamader bamader deleted the dev-dump branch November 26, 2024 13:36
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.

Add pgdump fileto run in dev mode
3 participants