Page MenuHomeSoftware Heritage

cli: Some cleanup and add missing options in db-init
ClosedPublic

Authored by anlambert on May 14 2019, 4:16 PM.

Details

Summary

A couple of cleanups for the main cli implementation.

I also added missing help option and log level processing in db-init subcommand

Diff Detail

Repository
rDCORE Foundations and core functionalities
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.May 14 2019, 4:16 PM
anlambert edited the summary of this revision. (Show Details)May 15 2019, 11:15 AM
anlambert updated this revision to Diff 4811.May 15 2019, 1:48 PM

Update:

  • cli: Remove use of private members from the logging module
  • cli.db: Activate help options and set log level
anlambert retitled this revision from cli: Modify log level option management to cli: Some cleanup and add missing options in db-init.May 15 2019, 1:50 PM
anlambert edited the summary of this revision. (Show Details)
anlambert updated this revision to Diff 4814.May 15 2019, 1:59 PM

Update: simplify code

Update:

  • cli: Remove use of private members from the logging module

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

  • cli.db: Activate help options and set log level

This one is indeed a good catch.

swh/core/cli/db.py
51–55 ↗(On Diff #4814)

This should not be needed there since the logging is set up in the main 'swh' group...

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

Maybe but this is a bad practice and should be avoided.

anlambert added inline comments.May 15 2019, 2:32 PM
swh/core/cli/db.py
51–55 ↗(On Diff #4814)

What do you mean ? Every sub-commands proceed like this.

anlambert added inline comments.May 15 2019, 2:35 PM
swh/core/cli/db.py
51–55 ↗(On Diff #4814)

Oh I see. If the log level is set to the root swh namespace, it will be inherited for sub ones ? This means we can remove the call to setLevel in subcommands ?

anlambert updated this revision to Diff 4815.May 15 2019, 2:41 PM

Update: no need to set the log level again in db-init

douardda added inline comments.May 15 2019, 2:42 PM
swh/core/cli/db.py
51–55 ↗(On Diff #4814)

yes. There are subcommands that do logging config stuff, but it is in general more than just a call to setLevel() (eg. in scheduler when calling setup_log_handler and so) but this is "just" some legacy parts that should be improved at some point IMHO.

anlambert updated this revision to Diff 4816.May 15 2019, 2:44 PM

Update: no need to pass click context in db-init

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

Maybe but this is a bad practice and should be avoided.

<slightly_bad_faith_ mode>like keeping a hand written copy of a piece of data that is in fact defined somewhere else </>

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

Maybe but this is a bad practice and should be avoided.

<slightly_bad_faith_ mode>like keeping a hand written copy of a piece of data that is in fact defined somewhere else </>

TBH both are "bad practice and should be avoided" but I am more reluctant to duplication than using "private" functions, AFAIC.

douardda resigned from this revision.May 15 2019, 2:51 PM

Let someone else give the thumbs up/down

Real word example of why using private members is a bad idea: https://github.com/Tulip-Dev/tulip/commit/cbb85d65da2cb262e0c20f2b9b16603b8a633aed#diff-675480f07686e76ad2b178ce3b9ccd4e

When Python 3.7 was released, some signatures in his private C API were updated.
We were using them to register internal Python modules written in C to an embedded interpreter (gdb did also the same, I found the proper fix from their mailing list) and thus compilation broke.

Ok this is a C example but the same could happen for Python code.

Considering the only duplicated thing is the name of the levels, this is not a big deal IMHO.

ardumont added subscribers: douardda, ardumont.EditedMay 15 2019, 3:13 PM

Considering the only duplicated thing is the name of the levels, this is not a big deal IMHO.

Will that prevent the cli interface from breaking (say against the day they remove the internal variable or change the content of that variable)?

@douardda Do you know why the authors make logging._nameToLevel private in the first place?

Will that prevent the cli interface from breaking (say against the day they remove the internal variable or change the content of that variable)?

Considering the log level names are standard and will not change, this will indeed prevent the cli from breaking if the logging module
implementation changes.

anlambert added inline comments.May 15 2019, 3:18 PM
swh/core/cli/__init__.py
10

I think the WARN level could be removed as it is not listed here https://docs.python.org/3/howto/logging.html#logging-levels

ardumont added inline comments.May 15 2019, 3:19 PM
swh/core/cli/__init__.py
10

yub yub

ardumont accepted this revision.May 15 2019, 3:20 PM
This revision is now accepted and ready to land.May 15 2019, 3:20 PM
anlambert updated this revision to Diff 4823.May 15 2019, 3:23 PM

Update: Remove the WARN entry from the levels list and sort it in ascending order by level priority

ardumont accepted this revision.May 15 2019, 3:26 PM
This revision was automatically updated to reflect the committed changes.

Real word example of why using private members is a bad idea: https://github.com/Tulip-Dev/tulip/commit/cbb85d65da2cb262e0c20f2b9b16603b8a633aed#diff-675480f07686e76ad2b178ce3b9ccd4e
When Python 3.7 was released, some signatures in his private C API were updated.
We were using them to register internal Python modules written in C to an embedded interpreter (gdb did also the same, I found the proper fix from their mailing list) and thus compilation broke.
Ok this is a C example but the same could happen for Python code.

We can find zillions of such examples for both 'bad practices', so this is no valid argument IMHO.
The risk here was that the usage of this private function breaks as a result of it being modified or killed, which would be immediately detected.

On the other side, having this hand written copy of the list of accepted/possible values that turns being out of sync because of such an evolution in the logging module, as well as not being able to cope with added log levels by 3rd party modules, is much harder to handle once it occurs.

We can find zillions of such examples for both 'bad practices', so this is no valid argument IMHO.

Yes it is, using private API or members can often lead to break builds or scripts execution.
So you will lose time to fix them and it could have been avoided by sticking to the public API
(sometimes you do not have a choice to implement some hacks I know but if there is any alternatives that
prevent the use of private things, it should be preferred)

On the other side, having this hand written copy of the list of accepted/possible values that turns being out of sync because of such an evolution in the logging module

Logging levels did not evolve between Python 2.7 [1] and Python 3.7 [2] so I think we are pretty safe here

as well as not being able to cope with added log levels by 3rd party modules, is much harder to handle once it occurs.

One solution to make everyone happy and handle all possible cases would be to use the following:

LOG_LEVEL_NAMES = [
    logging.getLevelName(v) for v in
    sorted(getattr(logging, '_levelToName', []))
] or ['NOTSET', 'DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']

[1] https://docs.python.org/2.7/library/logging.html#logging-levels
[2] https://docs.python.org/3/howto/logging.html#logging-levels