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

Enterprise falls back to PINT if no TEMPO2 #340

Open
wants to merge 30 commits into
base: dev
Choose a base branch
from

Conversation

aarchiba
Copy link
Contributor

@aarchiba aarchiba commented Feb 10, 2023

This has three significant changes:

  • If you run Pulsar(par, tim) without specifying PINT vs TEMPO2, one of two things will happen:
    • If you have TEMPO2 installed, you get a Tempo2 pulsar object as before.
    • If you don't have TEMPO2 installed, instead of an exception you now get a PintPulsar object.
  • Almost all tests now run on PintPulsar objects if TEMPO2 is not available; the test suite passes and testing is reasonably complete.
    • Tests that require TEMPO2 (for example the par file uses BINARY T2) are skipped.
    • Tests that fail with PINT (inflate/deflate) are marked as expected to fail (xfail).
    • A few tests try to use both PINT and TEMPO2, skipping the TEMPO2 version if it's not available. (These tests were already run with both.)
  • If PINT is not available, Enterprise continues to run using only TEMPO2 pulsar objects; the test suite likewise runs on almost all tests.

In short: you can use Enterprise transparently without TEMPO2. Also: a CI run now checks that Enterprise runs fine using PintPulsar for everything, and another checks that Enterprise runs fine without PINT. (Getting Enterprise to install when one or other dependency is missing is awkward but doable.)

@aarchiba
Copy link
Contributor Author

This is particularly valuable in light of #339 ; no seg faults if no TEMPO2.

@aarchiba
Copy link
Contributor Author

aarchiba commented Feb 10, 2023

Still to do:

  • Update CI so a run is done without TEMPO2 installed.

@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #340 (bcd3892) into master (5ef5ff4) will decrease coverage by 0.05%.
The diff coverage is 80.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #340      +/-   ##
==========================================
- Coverage   88.37%   88.33%   -0.05%     
==========================================
  Files          13       13              
  Lines        3012     3026      +14     
==========================================
+ Hits         2662     2673      +11     
- Misses        350      353       +3     
Impacted Files Coverage Δ
enterprise/pulsar.py 91.60% <80.00%> (-0.46%) ⬇️

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 5ef5ff4...bcd3892. Read the comment docs.

@aarchiba
Copy link
Contributor Author

Question: how do we make libstempo an optional dependency? The no-tempo run fails at pip install -r requirements_dev.txt because tempo2 isn't available (it didn't on my machine, for some reason). I believe I can fake this for now.

@aarchiba
Copy link
Contributor Author

The no-tempo2 installation is a bit of a hack, as we can't really tell pip "don't bother with this required dependency" and so it insists on installing libstempo, which fails if tempo2 is not available.

Long-term, we could make the tempo2 dependency optional - pip install enterprise[tempo2] - but we can't really make that the default (except on conda?).

Short-term, we might be able to trick libstempo into claiming to install even though tempo2 isn't available. This could perhaps be done most easily with a pip-install-fake-libstempo.py that creates a fake pure-python libstempo of the correct version and does pip install -e on that.

@aarchiba aarchiba changed the title WIP: Enterprise falls back to PINT if no TEMPO2 Enterprise falls back to PINT if no TEMPO2 Feb 12, 2023
@aarchiba
Copy link
Contributor Author

To get adequate coverage of the patch I added a few tests; several of them are XFAIL because the behaviour of Pulsar() is bad but I hesitate to fix it here. #332 also calls for improvements to Pulsar().

@aarchiba
Copy link
Contributor Author

We have three conditions for coverage:

  1. The overall coverage of the entire set of CI runs must meet some threshold implemented by codecov.
  2. The entire set of CI runs must cover some fraction of the lines in the PR, as implemented by codecov.
  3. Every individual CI run must cover at least 85% (now 75%) of the code base; this includes CI runs where PINT or TEMPO2 are not available.

I think the third requirement is awkward and inconvenient from a CI point of view, although it is one that can be checked locally with make test (unlike the other two). I'm not sure what the right solution to this is.

Perhaps we could have two makefile targets: make test runs with a local coverage requirement, while make test-ci still gathers coverage information but does not fail if this single CI run has insufficient coverage. The existing codecov setup will make the PR fail CI if the overall coverage is insufficient.

@aarchiba aarchiba changed the title Enterprise falls back to PINT if no TEMPO2 WIP: Enterprise falls back to PINT if no TEMPO2 Feb 13, 2023
@aarchiba
Copy link
Contributor Author

The changes to the code base are quite minor; just pulsar.py, mostly improved error handling. The bulk of the changes here are to the test suite, mostly just annotating tests with which program they need, but there are a few tests I had to convert to pytest style.

@aarchiba
Copy link
Contributor Author

I'm not sure what's up with the HTTP 404s - this occurs when astropy is trying to update its leap seconds file. It doesn't happen on my local machine, and it seems to only happen with the old python 3.7, pint 0.8.8, astropy 4.3.1 version; with astropy 5.2.1 the problem disappears. There were substantial upgrades to astropy's remote data fetching with astropy 5, so this may help explain why the problem only occurs with very old versions.

@aarchiba aarchiba changed the title WIP: Enterprise falls back to PINT if no TEMPO2 Enterprise falls back to PINT if no TEMPO2 Feb 14, 2023
@aarchiba
Copy link
Contributor Author

Something has happened to prevent the coverage upload from the "no tempo2" version. I'm not sure what; maybe a passing network problem.

@vhaasteren vhaasteren changed the base branch from master to dev November 7, 2023 09:02
@vhaasteren
Copy link
Member

vhaasteren commented Nov 10, 2023

I do not like the hoops we are jumping through here to have libstempo installed by default, but to allow for a no tempo2 version. Our situation is perhaps somewhat unique that we have two independent packages, neither of which is ideal:

  • Tempo2/libstempo are still a pain to install on some systems, despite progress in this regard
  • PINT is properly packaged, but slow, and newer so less well-supported

Unfortunately, Python's packaging system doesn't directly support conditional dependencies based on the presence of other packages at install time. The closest built-in feature is the extra requires functionality, with those square brackets.

My suggestion to deal with this in a consistent way would be to

  • Not have a required dependency on either libstempo or PINT
  • Provide good documentation on the need for one of the two (in case we want to use par and tim files)
  • Perform a runtime check for either of the two packages (and an associated warning)
  • Allow for Enterprise use without a timing package (e.g. PR Add MockPulsar class, for easy use of Mock data with Enterprise #361)

Of course the problem with this approach is that some build scripts and container scripts currently rely on libstempo being installed automagically, so if we make this change there will be things that break.

@vhaasteren vhaasteren mentioned this pull request Dec 9, 2024
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