Monitors setup migrations

[york-stsci]

This PR officially adds the monitors tables to the migrations folder, adds two columns to the NIRCam claw monitor, and filters out NIRCam claw images by date during the database query rather than after retrieval.

[mfixstsci]

Everything looks good to me @york-stsci thank you for this. Just a few questions and comments before I approve.

[bhilbert4]

LGTM

[BradleySappington]

Great work so far @york-stsci, I do have some feedback that we can discuss. Most of these comments are in the code review, but I want to consolidate them here with a few overall additions so my feedback is cohesive.

1. The MonitorRouter class should be in its own file called router.py and not associated with the common_monitor functionality. This router is a system wide addition (apologies, as this comment should have been addressed in the previous PR). I personally think router.py should live parallel to models.py however if you choose to keep it in the sub folder thats fine. I’ll continue my explanation assuming it is up a level for brevity.

2. We also need to set django up to see our router.py as part of its settings. Otherwise it will just sit there as a class. In order to do this we need to update jwql/website/jwql_proj/settings.py by adding the path to our router DATABASE_ROUTERS = ['jwql.router.monitor_router'] At this point, all db access defined in MonitorRouter should be automatically handled by django.

3. db_table_comment is being used functionally to associate models with databases. This is a clever workaround but not the intended use of this field. db_table_comment should be used “for documenting database tables for individuals with direct database access who may not be looking at your Django code.” We should put app_label back. I’m also not sure if app_label should be monitors or jwql, as we don’t have a separate monitors app associated with this repository (which is why I’d lean jwql).

4. If we aren’t using db_table_comment to align the models with their respective db, we should use the monitor model names from a list that should be defined in constants.py. This list will be accessed inside the MonitorRouter class. An example of how this would be done is:

from constants import monitor_models
def db_for_read(self, model, **hints):
    if model._meta.app_label == ‘jwql’:
        if model.__name__ in monitor_models:
                return ‘monitors’
    return None

Note: the above suggestion should be applied to all elements in the router, with the exception of allow_migrate which will use the parameter model_name instead of model.__name__

Happy to discuss further if you like!

[bsunnquist]

@york-stsci Look great! Only thing I see is to change the plot_type=='model' cell back to this:
if plot_type == 'model':
plot_data = df['median'].values / df['total_bkg'].values

[pep8speaks]

Hello @york-stsci, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2024-02-23 19:07:07 UTC

[BradleySappington]

Thanks Brian! Few small bugs I noticed

[york-stsci]

Note that, for databases other than dev, we might need to fake migration 17, because we can't use --fake-initial on any migration other than the first, and currently when you try to apply migration 17 it quite reasonably complains that the tables already exist.

[york-stsci]

And this is the part where I realize that I ran the migrations before updating the router class. Hang on whilst I rock the boat (i.e. roll back, then forward, repeat as required)

[york-stsci]

Logging this here for reference: It is entirely possible that migration 17 will fail when run with --database=monitors. If it does, then migration 17 (and only migration 17) should be run as migrate.py --fake when run with --database=monitors. This probably means we can't use the shell script, either on dev or production, in this specific case, because we have two migrations and may need to apply them one-at-a-time for one (but not both) database(s).

[mfixstsci]

I am good with this @BradleySappington seems happy as well as @bsunnquist. @york-stsci let me know when this is ready to merge.

[york-stsci]

I'd like @BradleySappington to take another look at the current form, just to backstop me, and then I think we're good.

[mfixstsci]

I'd like @BradleySappington to take another look at the current form, just to backstop me, and then I think we're good.

I think this is a great idea. @BradleySappington let me know when you are finished with your review and we can get this in!

[BradleySappington]

Great work Brian