search

spacetelescope / hst_notebooks

Generated Notebooks HTML
https://spacetelescope.github.io/hst_notebooks/
BSD 3-Clause "New" or "Revised" License
10 stars 15 forks source link

pin photutils to 1.12.0 for compatibility #296

Closed mrevalski closed 2 months ago

mrevalski commented 2 months ago

The large number of weekly CI failures are due to changes in photutils from 1.12 to 1.13, as described by Larry Bradley on Slack. Specifically, several function names have been changed, which we believe causes an issue with a custom defined DrizzlePac functions. This PR solves the issue by pinning photutils==1.12.0 for all DrizzlePac notebooks until updates can be completed.

Hi @mackjenn and @haticekaratay, I've opened this as a draft PR for discussion before merging. Do you both agree with the solution above? We can systematically test and update the photutils version in each notebook as time and resources allow.

mrevalski commented 2 months ago

Hi @haticekaratay, I realized this won't trigger a CI run because only the requirements.txt files were changed. Should I simply change e.g. an empty space in each notebook, or can you manually trigger the CI deployment to confirm all notebooks pass?

mrevalski commented 2 months ago

Hi @haticekaratay, I've confirmed with @mackjenn that this is a suitable solution for now.

Please review as your time allows, and let us know if you have any requested revisions.

Thank you, and have a great day! 🙂

haticekaratay commented 2 months ago

Hi @haticekaratay, I realized this won't trigger a CI run because only the requirements.txt files were changed. Should I simply change e.g. an empty space in each notebook, or can you manually trigger the CI deployment to confirm all notebooks pass?

Yes. The CI won't trigger the actions without an associated notebook. So as you suggested, let's add a small space to the markdowns.

review-notebook-app[bot] commented 2 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

mrevalski commented 2 months ago

Hi @haticekaratay, I realized this won't trigger a CI run because only the requirements.txt files were changed. Should I simply change e.g. an empty space in each notebook, or can you manually trigger the CI deployment to confirm all notebooks pass?

Yes. The CI won't trigger the actions without an associated notebook. So as you suggested, let's add a small space to the markdowns.

Hi @haticekaratay, I've tweaked a single comment in each notebook to trigger the CI. These changes are ready for review.

mrevalski commented 2 months ago

Hi @haticekaratay, note that the PEP8 errors are corrected in the pending PR #266 and related PRs.

All else looks good with the CI run. 👍

mrevalski commented 2 months ago

Perhaps we should split this into smaller PRs for each notebook to avoid issues with those having pending PRs?

haticekaratay commented 2 months ago

Perhaps we should split this into smaller PRs for each notebook to avoid issues with those having pending PRs?

No need. We can merge as is, and the authors could rebase their PRs on top of what we merge into the main.

haticekaratay commented 2 months ago

Perhaps we should split this into smaller PRs for each notebook to avoid issues with those having pending PRs?

No need. We can merge as is, and the authors could rebase their PRs on top of what we merge into the main.

A second thought, I think authors would find it challenging to rebase, as conflict resolution is harder for notebook files compared to any script. I will revert the changes to align_multiple_visits and sky_matching_notebooks.