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

Improve logging to make it easier to re-run with the same configuration #168

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Feb 23, 2024

Description

This PR introduces changes that make it easier to run the weekly agasc supplement update script with the same input arguments. This was motivated by #166

The current master version has all of the required features, except that the report date is not captured in the log file. With this PR, the report date is also included in the log.

As a convenience the list of AGASC IDs given to the script is also added to the log (also not kept in the log in the masters version).

Also added a few logging messages.

Interface impacts

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Independent check of unit tests by Jean

  • Linux
ska3-jeanconn-fido> pytest
============================= test session starts ==============================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: anyio-4.3.0, timeout-2.2.0
collected 72 items                                                                                                                

agasc/tests/test_agasc_1.py .....                                                                                           [  6%]
agasc/tests/test_agasc_2.py ..........................................                                                      [ 65%]
agasc/tests/test_agasc_healpix.py ...........                                                                               [ 80%]
agasc/tests/test_obs_status.py ..............                                                                               [100%]

================================================= 72 passed in 118.58s (0:01:58) ==================================================
ska3-jeanconn-fido> git rev-parse HEAD
4811f585cb067968426daa5672180e3067813d86

Functional tests

Follow these steps to run with the same arguments as last weeks processing:

mkdir pr-168
cd pr-168
cp /proj/sot/ska/data/agasc/rc/supplement_reports/weekly/latest/call_args.yml .
echo report_date: 2024:092:00:00:00.000 >> call_args.yml
# NOTE: edit call_args.yml to change output_dir and reports_dir so it does not overwrite stuff in $SKA/data
python -m agasc.scripts.update_mag_supplement --args-file call_args.yml

Base automatically changed from memory-fix to master February 23, 2024 18:09
@javierggt javierggt force-pushed the improve-log branch 2 times, most recently from 9e1d5d9 to 7920122 Compare April 3, 2024 15:24
@chandra-xray chandra-xray changed the base branch from master to commands-v2-again April 3, 2024 15:35
@@ -112,8 +113,7 @@ def get_parser():
return parser


def main():

def get_args():
logger = logging.getLogger("agasc.supplement")
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that a this chunk of "main" is now in get_args, maybe logger should just be global?

Copy link
Contributor Author

@javierggt javierggt Apr 4, 2024

Choose a reason for hiding this comment

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

It is not used anywhere else. I think it is better to not make things global until you have to.

# update 'bad' and 'obs' tables in supplement
agasc_ids += update_supplement.update(args)
# update 'bad' and 'obs' tables in supplement
agasc_ids += update_supplement.update(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this new indent on purpose? I would think you'd still want to do the update_supplement.update(args) even if you've been given list of agasc_ids in file_args, but maybe I'm missing a logical change here.

Copy link
Contributor Author

@javierggt javierggt Apr 4, 2024

Choose a reason for hiding this comment

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

Aha! good eye. Yes, I changed this on purpose, but I think you might have a point. I twas thinking of the agasc_ids. Also, if one is running with the same arguments, presumably this update to the supplement was already made, although one could be running with a copy of the supplement from the snapshot.

Note that there is another change. This change in indentation goes with the if "agasc_ids" in file_args: statement, which was not there to begin with, so everything after agasc_ids = [] is getting extra indentation.

This adds a feature that I did not document in the PR description. You can make a call_args.yml file by hand which includes the agasc_ids key, with a list of AGASC IDs. Note that there is no --agasc-ids command line argument, this is a feature of the call_args.yml file.

The truth is that we have never used the update_supplement part together with the update magnitudes part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change the indentation back. I don't think I have strong opinion on that, and maybe it is best to leave it as it was to make sure the same process is run.

@javierggt javierggt merged commit 70cd024 into commands-v2-again Apr 15, 2024
1 check passed
@javierggt javierggt deleted the improve-log branch April 15, 2024 14:38
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.

2 participants