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

allow per-instrument database table reset #1031

Merged
merged 12 commits into from Sep 26, 2022

Conversation

york-stsci
Copy link
Collaborator

This allows you to reset instrument tables on a per-instrument basis. Note that, when resetting tables per-instrument, anomaly tables will not be reset. When resetting the entire database, anomaly tables will be reset.

@pep8speaks
Copy link

pep8speaks commented Aug 26, 2022

Hello @york-stsci, Thank you for updating !

Line 471:82: W291 trailing whitespace
Line 478:88: W291 trailing whitespace
Line 483:77: W291 trailing whitespace

Line 27:16: E401 multiple imports on one line
Line 53:1: W293 blank line contains whitespace
Line 63:1: W293 blank line contains whitespace
Line 69:1: W293 blank line contains whitespace
Line 96:14: E261 at least two spaces before inline comment
Line 97:38: E261 at least two spaces before inline comment
Line 110:1: W293 blank line contains whitespace

Comment last updated at 2022-09-06 13:32:51 UTC

Copy link
Collaborator

@mfixstsci mfixstsci left a comment

Choose a reason for hiding this comment

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

Thanks @york, this looks great. Just had a quick comment and if you want to add the changes or have any disagreements with the suggestion, let me know what you think.

prompt = ('About to reset all tables for database instance {}. Do you '
'wish to proceed? (y/n)\n'.format(connection_string))
prompt = ('About to reset {} tables for database instance {}. Do you '
'wish to proceed? (y/n)\n'.format(instrument, connection_string))
response = input(prompt)

if response.lower() == 'y':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add something here for n and user input that aren't y/n?

if response.lower() == 'y':
    delete tables
elif response.lower() == 'n'
    print('selected n, exiting')
else:
    print("{} is not valid input, exiting".format(input)) 

Might be overkill, but besides that, this LGTM!

@bhilbert4
Copy link
Collaborator

As I was falling asleep last night, it occurred to me that we might want an option that will reset some subset of tables other than all tables for a given instrument. I was thinking about the cosmic ray monitor and what would happen if we e.g. changed the parameters for identifying a CR and then wanted to re-run just that monitor on everything. In that case we'd want to reset the CR monitor tables, but no others.

What do you think about adding an optional input that can take one or more table names, and then only those tables will be reset?

@mfixstsci
Copy link
Collaborator

As I was falling asleep last night, it occurred to me that we might want an option that will reset some subset of tables other than all tables for a given instrument. I was thinking about the cosmic ray monitor and what would happen if we e.g. changed the parameters for identifying a CR and then wanted to re-run just that monitor on everything. In that case we'd want to reset the CR monitor tables, but no others.

What do you think about adding an optional input that can take one or more table names, and then only those tables will be reset?

Maybe we could have an all option to delete everything or specify enter a list of tables?

@york-stsci
Copy link
Collaborator Author

What do you think about adding an optional input that can take one or more table names, and then only those tables
will be reset?

Maybe we could have an all option to delete everything or specify enter a list of tables?

At some point, we reach a level of detail where the answer probably should be doing things manually (i.e. by running python, importing jwql.database.database_interface, and then dropping tables from within that. I think my suggestion would be that we have 2 options, each of which takes a default "all", and an explicit boolean option that lets you add the anomaly tables, in case there's a reason you need to reset them (on the non-production databases):

  • instrument: all, miri, niriss, nircam, nirspec, fgs
  • monitor: all, bias, bad_pixel, dark, cosmic_ray, readnoise

So, if you did:

python reset_database.py -i miri -m dark

you would reset MIRIDarkQueryHistory, MIRIDarkPixelStats, and MIRIDarkDarkCurrent. If you did:

python reset_database.py -i fgs

you would reset FGSDarkQueryHistory, FGSDarkPixelStats, FGSDarkDarkCurrent, FGSBadPixelQueryHistory, FGSBadPixelStats, FGSReadnoiseQueryHistory, and FGSReadnoiseStats.

And, finally, if you did:

python reset_database.py --absolutely_everything

then you would get the initial confirmation question (are you sure you want to do this? y/N), and then you would get a message like:

WARNING: This command will reset every single table, including the anomaly tables. All existing anomaly data will be
permanently lost if you do this. Are you sure you want to proceed? [y/N]

and you would have to do "y" again to get that.

As another thought, if we're changing the options in any case, we can also add an explicit flag for using this on production, something like --run_on_production, which would act the same way, so that it doesn't just look to see if there's a 'p' in the hostname and then refuse to do anything, but so that also if you just use reset_database.py on production you would get an error like:

WARNING: You are trying to reset the databases on the JWQL production server. If you are sure that you want to do this,
run the command again with --run_on_production in order to reset the production tables.

@york-stsci
Copy link
Collaborator Author

Because ultimately what I'm assuming here is that we're not trying to guard against people trying to do the wrong thing, we're instead trying to guard against people accidentally running a different command than they intended, and so ultimately what we most want to do is have a standard workflow, where you run the script and enter 'y' once, for any of the things you would typically want to do (reset some non-anomaly tables on either test or dev), but if you want to do one of the more dangerous options (resetting anomaly tables or running on production) then there's an explicitly different workflow, which in turn means that we hopefully jolt people out of accidentally doing it on automatic (or without checking which server they're logged in to), and make them think. And, of course, keeping the existing system where, if you just hit enter, it's the same as hitting "no", helps to prevent accidents as well.

@york-stsci
Copy link
Collaborator Author

Just as a note, I've now done some testing of this on the dev server. In particular, I've verified that

  • If you enter anything other than y/Y at the prompts, it quits without changing the databases
  • If you try to delete an anomaly table without explicitly adding the anomaly table command line flag, it quits without changing the databases
  • If you ask it to reset a particular instrument, then only tables related to that instrument are emptied
  • If you ask it to reset a particular monitor, same

For obvious reasons, I'm not testing on whether you can run it on production, but the production flag works exactly the same way as the anomaly flag (i.e. you have to specifically use the "really really reset production" command-line flag in order for it not to just quit). But I think it might already be at the point where it's useful. Certainly, being able to reset the bias tables without touching the other monitors was useful for my ongoing testing work.

@york-stsci
Copy link
Collaborator Author

@bhilbert4 @mfixstsci I think this is at the ready-for-review-or-merge stage. I've been actively using it to reset parts of the dev database as an active part of my last bits of celery testing.

For reference, here's the text you now get if you do "reset_database.py --help":

usage: reset_database.py [-h] [-i INSTRUMENT] [-m MONITOR] [--explicitly_reset_production] [--explicitly_reset_anomalies]

Reset JWQL database tables

optional arguments:
  -h, --help            show this help message and exit
  -i INSTRUMENT, --instrument INSTRUMENT
                        Instrument tables to reset ('all' for all), default 'all'
  -m MONITOR, --monitor MONITOR
                        Monitor tables to reset ('all' for all), default 'all'
  --explicitly_reset_production
                        Needed to allow reset of Production tables
  --explicitly_reset_anomalies
                        Needed to allow reset of anomaly tables

@mfixstsci mfixstsci merged commit 6596527 into spacetelescope:develop Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants