Skip to content

Conversation

@sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Jun 5, 2020

Closes #37. Sister PR to NCAS-CMS/cfdm#34, with largely the same purpose & implementation, & using much of the corresponding functionality there (by import where possible) but with the following key differences:

  • cf-python applies info & _debug keywords as well as just verbose to enable filtering on some level of verbosity. These keywords have all been converted to verbose for consistency, & appropriate level have been given to any call being filtered as such, e.g:

    if _debug:
        print("my message")

    has been changed to the following, so it only emerges with debug logging configured locally via verbose keyword (see next point), or if not globally:

    logger.debug("my message")

    where info levels have been mapped appropriately, as discussed, namely:

    Min. verbosity (Old) info level New verbose level
    warning 0 1
    info 1 2
    detail 2 3
    debug 3 -1
  • unlike in cfdm there are cases where the verbosity is specified not via a function keywords arguments, but via an instance attribute e.g. self.verbose (or, pre-conversion, self.info). To manage these I have created another decorator which is analagous to the existing _manage_log_level_via_verbosity but adapts the log level for runs of decorated methods according to the value of the verbose as an attribute instead as a keyword argument.

Work still TODO for this PR

Currently a WIP with functionality largely there but docs & testing pending:

  • apply deprecation messages on info & _debug where appropriate;
  • adapt test verbose specifications;
  • add docs section;
  • adapt docstrings;
  • double check format conversions from multi-argument print statements;
  • add explicit assetLog-based tests;
  • manual testing in anger (esp. check interfacing with cfdm logging);
  • add new function to API ref;
  • update changelog;
  • address DH feedback on docstring formatting.

Note: setup and configuration is identical to that in cfdm, except
cf-python uses its own LOG_LEVEL constant in the CONSTANTS
dictionary to manage the log level (not the one from cfdm).

Includes loosening of the cfdm version contraint from an exact to a
minimum version, as agreed previously (changed now for development).
Includes application to aggregate module via _Meta class. Tests to
come in next commit.
It may not be workable in these cases due to complications so we
can use the global LOG_LEVEL to manage these instead (at least for
this release).
From testing it is not simple to add verbosity as an instance
attribute where it was not an equivalent attribute before (e.g.
self.info) due to coupling etc. Abadon idea to extend. The global
cf.LOG_LEVEL can manage message filtering inside those classes.
@sadielbartholomew
Copy link
Member Author

I will address the two outstanding tasks while doing the release & awaiting the tests to complete for that.

Opening now to see how bad the merge conflicts are via the UI. Then I will see how the CI tests fare. I am seeing a sub-test error locally (test_Datetime_Data (__main__.DatetimeTest) ... ERROR) but I suspect it is because I need to update my branch so it includes everything that has since gone into master.

@sadielbartholomew sadielbartholomew marked this pull request as ready for review June 8, 2020 22:38
@sadielbartholomew
Copy link
Member Author

Conflicts resolved, now closing & re-opening to force the GH Actions checks to start.

@sadielbartholomew
Copy link
Member Author

Trying the CI checks again, & I have added an install of pycodestyle so the corresponding, & new, style test can run via Actions.

@sadielbartholomew
Copy link
Member Author

sadielbartholomew commented Jun 9, 2020

This PR has now become muddied with some pre-release commits, notably PEP8 style fix lines & UM read tweaks to try to get some of the test failures to pass. I should really have done these post-PR but I did not think ahead.

Running the tests again, as I was trying to pinpoint which test was giving a full debug log dump & think I have sorted that (it was due to updates that had gone into master & needed amending for the logging logic after incorporation here).

@sadielbartholomew
Copy link
Member Author

Final check before merge. I expect the test_PP_* tests to fail, as there is clearly an issue with the GH Actions environment/setup in this respect. Those tests pass locally so it is fine.

@sadielbartholomew
Copy link
Member Author

Just the three test_PP_* tests erroring as expected, so now good to merge.

@sadielbartholomew sadielbartholomew merged commit 8aa0e05 into NCAS-CMS:master Jun 9, 2020
@sadielbartholomew sadielbartholomew deleted the add-logging branch June 10, 2020 18:11
@sadielbartholomew sadielbartholomew mentioned this pull request Jun 15, 2020
3 tasks
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.

Add logging (replace '_debug' & more) pre-parallelisation

2 participants