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

CI: execute for rendering #309

Merged
merged 9 commits into from
Sep 14, 2024
Merged

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Aug 20, 2024

I cannot test this locally due to some install issues for healpy, so will use CI to iron out the issue. I'm sorry for the notification spams for those of you watching this repo.

We need to handle dependencies sensibly.

These items are considered blockers:

Upstream issues, not blockers for this PR

  • if possible add all of them to the image we use

Whishlist items, should not block merging

  • have failing job status when there are tracebacks in rendering. (It should still generate the HTML outputs so we are able to check things, but the job status should be a red one)

@bsipocz bsipocz added the infrastructure Issues about the infrastructure rather than content of the notebooks label Aug 20, 2024
@bsipocz bsipocz marked this pull request as draft August 20, 2024 20:02
@bsipocz
Copy link
Member Author

bsipocz commented Aug 20, 2024

Current status:

  • 🔴 All have issues with the install/import cells mentioned above

  • 🔴 Forced photometry:

    • Errors out with:
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
Cell In[23], line 2
     1 #proove we can do this for one object
----> 2 calc_instrflux(*paramlist[0][1:])
     4 #same thing, different syntax
     5 # calc_instrflux(paramlist[0][1], paramlist[0][2], paramlist[0][3], paramlist[0][4], paramlist[0][5], paramlist[0][6])

Cell In[21], line 54, in calc_instrflux(band, ra, dec, stype, ks_flux_aper2, img_pair, df)
    49 subimage, bkgsubimage, x1, y1, subimage_wcs = cutout.extract_pair(
    50     ra, dec, img_pair=img_pair, cutout_width=band.cutout_width, mosaic_pix_scale=band.mosaic_pix_scale
    51 )
    53 # find nearby sources that are possible contaminants
---> 54 objsrc, nconfsrcs = find_nconfsources(
    55     ra, dec, stype, ks_flux_aper2, x1, y1, band.cutout_width, subimage_wcs, df
    56 )
    58 # estimate the background
    59 skymean, skynoise = photometry.calc_background(bkgsubimage=bkgsubimage)

NameError: name 'find_nconfsources' is not defined
  • 🔴 Light curve generator
    • Errors out with:
AttributeError: module 'numpy' has no attribute 'msort'
  • 🟠 Light curve generate, large scale

    • Works, but rendering is really ugly. Too many progressbar and other broken line wrappings, etc.
  • 🟢 Light curve classifier

    • Looks reasonable
  • 🔴 AGN Zoo

    • errors out with
AttributeError: `np.Inf` was removed in the NumPy 2.0 release. Use `np.inf` instead.

@troyraen
Copy link
Contributor

I cannot test this locally due to some install issues for healpy

Do you know where the healpy requirement is coming from? I searched the whole repo but see no mention of it. I had thought we already switched everything to use hpgeom instead. (And we switched because of the install issues with healpy.)

@bsipocz
Copy link
Member Author

bsipocz commented Aug 20, 2024

Do you know where the healpy requirement is coming from?

Yes, it's coming from nway. The issue is that there are no wheels for older OSX with ARM chips, thus it tries to build it from the source and fails. I'm sure I could fix it by diving into the errors, but instead pinged the developers if they are willing to release more wheels

(Using CI to fix issues is I feel legit here, as we don't really need to support OSX and Windows for these notebooks; though this/similar issue will come up with lsdb/hipscat too when those pull in healpy as a dependency).

@zoghbi-a
Copy link
Contributor

zoghbi-a commented Aug 22, 2024

@bsipocz, how about using our fornax images in the CI? I did a little experiment to test that it can be done in this test-actions repo. It seems to work fine.
It pulls the images from the public aws ECR (in this case, tractor-develop) and use docker to run code inside the image.

A few things may not run because we are outside the fornax system (e.g. restricted buckets etc.), but we can make the notebooks robust against that as part of their portability.

@bsipocz
Copy link
Member Author

bsipocz commented Aug 22, 2024

how about using our fornax images in the CI

Yes, I think this is a sensible idea, we may already have an issue about it. So far all the issues I run into while doing this PR are unrelated to the image (and e.g. the missing install of tractor; besides that I don't expect any image related problems).

@bsipocz bsipocz force-pushed the CI_execute_for_rendering branch from 4799e0e to 7ab0bd7 Compare August 27, 2024 02:19
@bsipocz
Copy link
Member Author

bsipocz commented Aug 27, 2024

Now this is placed on top of #318 and #321

@bsipocz
Copy link
Member Author

bsipocz commented Aug 27, 2024

The ML_AGNzoo notebook now runs into a file access issue: #322

@bsipocz bsipocz linked an issue Aug 28, 2024 that may be closed by this pull request
@bsipocz
Copy link
Member Author

bsipocz commented Aug 28, 2024

Given #324, I'm adding that into the exclude list, too. So the scope of this PR will be scaled back to:

  • have execution infrastructure in place
  • perform the execution for those notebooks that work out of the box
  • leave all problematic ones out rather than hold back merging this.

I will also propose to reshuffle the commits between this and #321, so all non-controversial cleanup commits can land in that one and be merged asap.

@bsipocz bsipocz force-pushed the CI_execute_for_rendering branch from 690b8b2 to f7c7ea1 Compare September 13, 2024 00:00
@bsipocz bsipocz force-pushed the CI_execute_for_rendering branch from f7c7ea1 to 49cbfab Compare September 13, 2024 00:01
@bsipocz bsipocz marked this pull request as ready for review September 14, 2024 00:41
@bsipocz
Copy link
Member Author

bsipocz commented Sep 14, 2024

The latest commit of da1ccc1 should close #331

@bsipocz bsipocz linked an issue Sep 14, 2024 that may be closed by this pull request
Copy link
Member Author

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

OK, let's have this in even though the rendered content still has some issues (==looks ugly due to the progress bars and other too verbose logging in the scaled light curve generator). However, the classifier looks reasonably good (and the rest are skipped for various problems, all of which has their own issues opened).

@bsipocz bsipocz merged commit 0434f1e into nasa-fornax:main Sep 14, 2024
3 checks passed
@bsipocz bsipocz deleted the CI_execute_for_rendering branch September 14, 2024 03:54
github-actions bot pushed a commit that referenced this pull request Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTML rendering infrastructure Issues about the infrastructure rather than content of the notebooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: kernelspec metadata incompatibility between FSC and CI/local CI: add circleCI rendering job for PRs
3 participants