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

Add missing 3.11 test command #1808

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

jhunkeler
Copy link
Contributor

Fixes:

  • HCALDP pipeline job not testing anything under Python 3.11
  • Regression tests not running under Python 3.11

@jhunkeler jhunkeler requested review from mdlpstsci and a team as code owners June 6, 2024 14:07
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 31.31%. Comparing base (92cfb36) to head (fcf98c9).

Current head fcf98c9 differs from pull request most recent head e7e96e7

Please upload reports for the commit e7e96e7 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1808      +/-   ##
==========================================
- Coverage   33.18%   31.31%   -1.87%     
==========================================
  Files         126      158      +32     
  Lines       31164    34948    +3784     
  Branches     5778        0    -5778     
==========================================
+ Hits        10341    10945     +604     
- Misses      19654    24003    +4349     
+ Partials     1169        0    -1169     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding the tests to be run under Python 3.11.

However, I do have some questions regarding this file where the build configs all migrate up a version of Python. By this I mean:
Should the Python 3.10 builds just become Python 3.11 builds?
Should there be a section added to run the Python 3.11 build under MacOS?
Should we add a section for Python 3.12?

Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should lines 47, 49, and 52 be "b2"? instead of "b1"?

Copy link
Collaborator

@mdlpstsci mdlpstsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! This version of the JenkinsfileRT is a model template.

@s-goldman
Copy link
Collaborator

s-goldman commented Jun 7, 2024

I previously saw that the bc1.failedFailureThresh line is duplicated at lines 37 and 52. I thought that it may have been a mistake, and the second mention should instead be a command for bc2. The second bc1.failedFailureThresh = 1000 command needs to be there or the tests fail.

You may have fixed that with these improvements, but I just wanted to make sure that someone else doesn't make my mistake.

@s-goldman
Copy link
Collaborator

Reintroducing Mac tests may result in some failures. Previously, the differences between results produced by the two architectures (linux and mac) are larger than the current allowable tolerances in the tests. Before we implement these changes we need to make sure that they pass.

@s-goldman
Copy link
Collaborator

s-goldman commented Jun 7, 2024

This is also a side request: It would be great if the linux architecture that we use for testing on jenkins is as close to possible to servers that we have access to (e.g. dlhlalab#). This would really help with debugging. Currently, when the tests are failing on jenkins and no where else, we don't really have a means of debugging the jenkins tests.

I know we are moving away from jenkins, but I figured it might be an easy change.

@mdlpstsci
Copy link
Collaborator

mdlpstsci commented Jun 7, 2024

The Linux and MacOS tests are now both passing. The problem which I recall causing discrepancies between the Linux and MacOS results resided in the source catalogs, specifically the number of found sources. Currently, those particular tests are skipped while we are doing some basic tests on the catalogs. When explicitly and manually testing on Linux and MacOS, I got the same results. The problem that I could not see what was happening on the test/build machines to figure out the issue. Since I am currently modifying the catalogs a great deal, I am expecting the new results to be very different from the current assert values. I will be generating new values after I am done with changes, as well as running tests under Linux and MacOS.

@mdlpstsci
Copy link
Collaborator

mdlpstsci commented Jun 7, 2024

@s-goldman @jhunkeler

https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2Fdrizzlepac-new-rt/detail/drizzlepac-new-rt/4/tests/

I have been looking at the output failures and found some quick and some not so quick issues to address. The quick fixes are:

  1. Set the crrefer environment variable (Joe has done that in this ticket)
  2. Recover erroneously deleted poller files needed for the tests (Michele has done this in Git PR#1809).
  3. Tests test_svm_canary.py and test_svm_hrcsbc.py are effectively the same. One of the tests should be deleted (test_svm_canary.py). (Michele also did this in Git PR#1809).

It should be noted that once the tests which failed due to the missing crrefer variable execute, the output products could show differences.

Not so quick issues:
As @s-goldman noted correctly earlier there are tests (image tests) which pass on Linux and DO NOT pass on MacOS. These need to be investigated in a different ticket.

  1. tests/acs/test_tweak.py and tests/acs/test_unit.py fail due to results exceeding difference tolerances or some results are just very different.
  2. tests/hap/test_svm_hrcsbc.py no longer achieves a FIT_SVM_GAIAeDR3 WCS solution.

@jhunkeler jhunkeler force-pushed the fix-missing-test-commands branch 3 times, most recently from e7e96e7 to 9cf5cc1 Compare June 14, 2024 13:52
@mdlpstsci
Copy link
Collaborator

@jhunkeler Do we tempt fate by pushing this ticket? Just curious. Also, the drizzlepac docs seem to be failing on most PRs due to a bad path (see below). Will these changes accommodate this issues?

CondaVerificationError: The package for sphinx located at /home/docs/.asdf/installs/python/mambaforge-4.10.3-10/pkgs/sphinx-7.3.7-py311h5eee18b_0
appears to be corrupted. The path 'lib/python3.11/site-packages/sphinx/util/rst.py'
specified in the package manifest cannot be found.

* Add proper installation command to RT test_cmds
* Refactored the coverage_install variable as coverage_exec
* Remove independent call to ./coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants