askap-vast / vast-pipeline

This repository holds the code of the Radio Transient detection pipeline for the VAST project.
https://vast-survey.org/vast-pipeline/
MIT License
7 stars 3 forks source link

Add date and time stamp to log files #595

Closed ajstewart closed 2 years ago

ajstewart commented 2 years ago

@marxide here is a fix for the log files having no time stamp. I think you mentioned previously that maybe the log files should be added to the DB to avoid the glob. This is a glob version so I thought I'd put it up in case you want to use this as a base to do that.

To do:

Fixes #593.

ajstewart commented 2 years ago

Looks good, but won't this break existing runs? I think we'd need to add in a glob for the original log.txt if none with timestamps are found.

Or run a script to rename the logs for existing runs. It could extract and add the timestamp from the first line of the log to the filename. While not a database operation, it might be suitable to put in a manually created data migration (i.e. using RunPython) so that it's clear it needs to be run. https://docs.djangoproject.com/en/4.0/topics/migrations/#data-migrations

Ah yes I meant to comment that it wasn't backwards compatible. I don't mind either way, adding log.txt to the run log glob is a simple way to maintain it.

With the migration you mean to just display a note that run logs need to be updated? Or attempt an automated update method?

ajstewart commented 2 years ago

@marxide added support to display the old logs in https://github.com/askap-vast/vast-pipeline/pull/595/commits/c3017f150c02aaa1c9789175c2b369f1bf506a6e.

marxide commented 2 years ago

With the migration you mean to just display a note that run logs need to be updated? Or attempt an automated update method?

No, I meant that you can have arbitrary Python functions executed during a Django migration. They are meant for manipulating existing records in the database to suit the new schema changes in the migration, but there's nothing stopping us from writing one of these data migrations ourselves that rename log files. Putting it in as a migration just means it will be executed when python manage.py migrate is run, which we do after we deploy an update.

Including the exising log.txt in the glob should be fine.

ajstewart commented 2 years ago

@marxide I've added a migration step to convert old logs, I agree in that it's a neater solution for the old ones.

marxide commented 2 years ago

Thanks for doing the migration, that will make updating easier!