Skip to content

Conversation

@mauch
Copy link
Contributor

@mauch mauch commented Jan 24, 2025

Mostly based on #707 but with changes to refect more recent changes to the pipeline:

  • Updated delete_run_raw_sql to deal with UUIDs
  • Some minor bugfixes from original implementation in v2.0 Experiment: Add Django PostgreSQL copy support (3/4) #707 (eg. initialise mem_csv in copy_upload_model, make object_id a UUID in CommentModel)
  • UUIDs broke the prev and next buttons in the web interface so I fixed these, and made minor changes to the text in the web interface (ID number -> ID)
  • Fixed a bug with delete_run_raw_sql where multiple sources using the same tag cause the delete to crash (The fix was to only delete a tag when there are no more database sources referenceing it)
  • Updated migrations to install q3c into the SQL database.

1. Need to see if object_id in Comment model needs to be UUID so we can delet using raw SQL on that rather than id
2. The associations_df upload and prep for it might be reverted to behaviour from pre #787 - will have to check and update if necessary
3. Need to set up q3c migrations for the database automatically.
4. Need to merge #800 into this so that vaex doesn't break everything.

@mauch
Copy link
Contributor Author

mauch commented Jan 31, 2025

Maybe just truncation to a certain number of characters, along with removing hyphens?

Alright. Ill take a closer look on Monday.

@ddobie
Copy link
Contributor

ddobie commented Feb 4, 2025

Let's leave the UUID character set stuff as-is for now and I can tweak the details later. I've opened an issue for that: #807.

As discussed yesterday, let's go with a "short" option (5 characters) for runs, images etc. medium (12 characters) for sources and long (15 characters) for measurements.

Copy link
Contributor

@ddobie ddobie 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 to me at this stage, although I suspect you're still working on things

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check, the migrations file is automatically generated? So even though the character sets and lengths are hardcoded in here, they're functionally not hardcoded anywhere except where they're defined?

Copy link
Contributor Author

@mauch mauch Feb 6, 2025

Choose a reason for hiding this comment

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

Just to double check, the migrations file is automatically generated? So even though the character sets and lengths are hardcoded in here, they're functionally not hardcoded anywhere except where they're defined?

They are defined here: https://github.com/askap-vast/vast-pipeline/blob/UUID_indexes/vast_pipeline/utils/utils.py#L28

then they are put into the models here (e.g.): https://github.com/askap-vast/vast-pipeline/blob/UUID_indexes/vast_pipeline/models.py#L110

The migrations are created from the models using migrate.py makemigrations so if you change the character set in utils.py you need to update the migrations as well with that command (Ideally you aren't going to be updating the migrations very often!).

And likely only when you start a new database from scratch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also -there are some hardcoded source strings in the test .csv files so those would also have to change if you changed the string length.

Copy link
Contributor

@ddobie ddobie left a comment

Choose a reason for hiding this comment

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

Good to go - just a minor typo in the comments

@mauch mauch merged commit 315df0f into v2.0 Feb 6, 2025
4 checks passed
@mauch mauch deleted the UUID_indexes branch February 6, 2025 03:37
@mauch mauch restored the UUID_indexes branch March 29, 2025 20:11
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.

4 participants