ledatelescope / bifrost

A stream processing framework for high-throughput applications.
BSD 3-Clause "New" or "Revised" License
64 stars 29 forks source link

Support `python -m bifrost.version` command #163

Closed league closed 2 years ago

league commented 2 years ago

Just enables a simple command to print out version and copyright info:

% python -m bifrost.version
bifrost 0.9.0.dev+g2bc27cc
Copyright (c) 2016-2020, The Bifrost Authors. All rights reserved.
Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
License: BSD 3-Clause

I think it would be nice to offer many of the 'tool' scripts with this interface, e.g. python -m bifrost.telemetry --enable rather than (or I guess in addition to) a script file in bin/.

codecov-commenter commented 2 years ago

Codecov Report

Merging #163 (ef74ece) into autoconf (54ed45b) will decrease coverage by 1.14%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           autoconf     #163      +/-   ##
============================================
- Coverage     59.01%   57.86%   -1.15%     
============================================
  Files            65       68       +3     
  Lines          5673     5898     +225     
============================================
+ Hits           3348     3413      +65     
- Misses         2325     2485     +160     
Impacted Files Coverage Δ
python/bifrost/__init__.py 80.00% <0.00%> (-0.77%) :arrow_down:
python/bifrost/version/__main__.py 0.00% <0.00%> (ø)
python/bifrost/telemetry/__init__.py 35.83% <0.00%> (ø)
python/bifrost/telemetry/__main__.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 54ed45b...ef74ece. Read the comment docs.

jaycedowell commented 2 years ago

It looks like there is also a reference to version.py in bifrost/__init__.py.

jaycedowell commented 2 years ago

I took a look through tools and telemetry is the only one that I see that would be a good candidate for the python -m ... treatment.

league commented 2 years ago

When you did the python -m treatment for LSL, you didn't encounter this message:

RuntimeWarning: 'BLAH.__main__' found in sys.modules after import of package 'BLAH',
but prior to execution of 'BLAH.__main__'; this may result in unpredictable behaviour

It looks like you still have just one module lsl.misc.telemetry that includes an if __main__ block. When I tried it that way for Bifrost telemetry (or version), I got the above warning. Getting around it seemed to mean using a separate __init__.py for the module definitions and then __main__.py for the program. And I think it's precisely because telemetry is imported everywhere (and version is imported by the bifrost main module). In general, if the module is not already imported before you try to run it, having an if __main__ block will be fine.

Or having the script live at bifrost.tools.telemetry but the module imported everywhere is bifrost.telemetry, which is less satisfying.

jaycedowell commented 2 years ago

I didn't get that warning but I also have extensively tested my changes.

jaycedowell commented 2 years ago

After trying a little harder I do get that warning but only under Py3.

jaycedowell commented 2 years ago

Ok, well, that's fixed in LSL.

Do you want to go ahead and expand this to cover bifrost.telemetry? I could tackle that one if it would help.

league commented 2 years ago

@jaycedowell: I believe I have a "git stash" where -m bifrost.telemetry more-or-less works, so I could probably push that to the autoconf branch. However, for the purpose of building documentation (and possibly test suites etc.) I have a minor revision of telemetry.py on commit 6ad2617c1081651a6d832b35897a6e009b00217d that would be nice to include – or just address the conflict when I'm ready to merge #162... either way I guess.

If you think 6ad2617 is fine, I could cherry-pick that onto autoconf, fix its conflict with my stashed telemetry-main, and commit.

league commented 2 years ago

Update: I see you did something similar to telemetry in lwa-project/lsl@0d12197cfe7ef2589bbc05ce9f95c7f95594890a.

jaycedowell commented 2 years ago

Yeah, I think that is fine. The alternative to disabling the telemetry would be to generate a new _INSTALL_KEY each time and report with that. I don't think that is necessary since this readonly condition seems like an edge case.

jaycedowell commented 2 years ago

Thinking about your comment in #164, what about doing something like having python -m bifrost.version give you what it does now and then adding something like a python -m bifrost.version --config that gives you that, plus how the library is configured. That might be helpful for people who are on a system where Bifrost is already installed and they might be curious how it is configured.

league commented 2 years ago

Yes! That looks very helpful. Small friendly amendment forthcoming… haha.

jaycedowell commented 2 years ago

Does this need anything else or is it ready to merge?

league commented 2 years ago

Ready to go.