H4cv2 node

[david-deboer]

This is the new version consistent with the new node_control (so online testing needs node_control default set to main).

Many changes...

[bhazelton]

You can update the yaml in this branch to explicitly point to the main branch of the node_control repo, it doesn't have to be controlled by which branch is the default. see https://stackoverflow.com/questions/20101834/pip-install-from-git-repo-branch

[codecov[bot]]

Codecov Report

Merging #561 (9be57ac) into main (bc4d57e) will increase coverage by 0.31%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #561      +/-   ##
==========================================
+ Coverage   97.38%   97.70%   +0.31%     
==========================================
  Files          35       35              
  Lines        5125     5097      -28     
==========================================
- Hits         4991     4980      -11     
+ Misses        134      117      -17     

Impacted Files	Coverage Δ
hera_mc/__init__.py	86.36% <100.00%> (+0.64%)	:arrow_up:
hera_mc/cm_active.py	100.00% <100.00%> (ø)
hera_mc/cm_dossier.py	95.78% <100.00%> (ø)
hera_mc/cm_handling.py	100.00% <100.00%> (ø)
hera_mc/cm_hookup.py	100.00% <100.00%> (ø)
hera_mc/cm_partconnect.py	97.93% <100.00%> (ø)
hera_mc/cm_redis_corr.py	100.00% <100.00%> (ø)
hera_mc/cm_revisions.py	100.00% <100.00%> (ø)
hera_mc/cm_sysutils.py	100.00% <100.00%> (ø)
hera_mc/cm_utils.py	100.00% <100.00%> (ø)
... and 8 more

---

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 bc4d57e...9be57ac. Read the comment docs.

[bhazelton]

In order to understand some of the issues we're talking about, I checked out this branch. I discovered when I tried to run the tests that you have to have hera_node_mc installed and a redis db setup to run the tests. That's not our standard practice, we want people to be able to run the tests without errors even if they only have the things listed in setup.py installed. We typically have importer skips to automatically skip tests that require extra packages.

See, e.g. the requires_redis decorator in hera_mc/tests/__init__.py

I see that we actually have a mixture of patterns on this front. I think we can leave this alone for now, but I'm going to make an issue to add a min_deps ci step to make sure people can run the tests with just the packages listed in setup.py.

[bhazelton]

I think the concern is that if you default it it can allow incorrect things to be passed without the user's knowledge. But there may be cases where it makes sense and is unlikely to cause real problems. In other parts of the hera_mc codebase that I wrote, users just have to pass in an astropy Time object, so I guess I tend to go the other direction to really force users to be explicit.

I just noticed that a few lines lost coverage with some of the work I did to explicitly pass in the redis hostname in all the tests. I'd like to fix those, I'm mid-process on those fixes.

[david-deboer]

I would favor leaving it as None. If you put in a default, then you have a 2/3 of getting it wrong ;-)

[bhazelton]

@plaplant I added the coverage I wanted to, I think this is ready for a re-review when you have time.