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

ParSNIP Cron Deployment #311

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Conversation

patrickaleo
Copy link
Contributor

Description of the Change

ParSNIP Cron.

I've added parsnip_classify.py, which runs the cron "parsnip_classify_cron". It needs the ParSNIP RFC model files and the simulation model file, which are sent via Box (message me if you haven't gotten them). You define the path to these models in settings.ini, in the new parsnip section. parsnip_classify.py will load them from settings.ini automatically. I've added dummy variables to the public-facing public_settings.ini.

I've uploaded a requirements.web.dev. I've successfully built (but not tested) an image using docker-compose -f docker-compose.dev.yaml up --build. However, note that I technically use numpy==1.20.3 and astropy==4.2.1. I've added some leeway with the versions, so if this breaks anything, change the requirements file to these exact versions. My exact process to run these files locally were:

  1. pip install --upgrade pip
  2. pip install astro-parsnip==1.3.1
  3. pip install onnxruntime (this upgrades numpy to 1.24.1 which has asscalar bug, so)
  4. pip uninstall numpy
  5. pip install numpy==1.20.3 (the version before)
  6. pip uninstall astropy
  7. pip install astropy==4.2.1
  8. python3 manage.py runcrons YSE_App.parsnip.parsnip_classify.parsnip_classify_cron --force

Note that in models/ I've modified files to allow for the new attribute: t.photo_class_conf, the confidence (%) association with the photometric classification t.photo_class (which should replace/overwrite the old RAPID t.photo_class).

Lastly, note that the cron automatically runs ONLY the 3-type (SN Ia/ SN II/ SN Ibc) model, not the 2-type model.

Checklist

Please check all that apply to your proposed changes

  • HTML code change
  • Added package dependency
  • Added data
  • Changed django model
  • Documentation change

Additional context

@patrickaleo patrickaleo changed the base branch from master to develop March 22, 2023 02:36
Copy link
Collaborator

@astrophpeter astrophpeter left a comment

Choose a reason for hiding this comment

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

Hey Patrick, looks like the docker image is building in the CI now and works! Could you please confirm that the make migrations works locally? So checkout a fresh version of this branch and a fresh yse-pz database backup and run make migrations and migrate to see if there are any issues?

@astrophpeter
Copy link
Collaborator

@davecoulter Can you please take a look? make migrates and migrate appear to work with a fresh checkout according to Patrick :)

@astrophpeter astrophpeter self-requested a review March 28, 2023 18:43
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just white space changes to this file, please remove!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove white space changes to this file, and we need the migrations relating to these model changes to be checked in if we want to deploy to test.

@davecoulter
Copy link
Owner

davecoulter commented Mar 28, 2023

Hi @patrickaleo -- good work overall -- however, there are a couple of things I'd love to see addressed. While @astrophpeter has left file-specific comments, I will reiterate them here for clarity:

  1. Files that have only white space changes should just be reverted instead of checked in. Specifically, tag_models.py. There's no change in there so let's roll it back.

  2. For the transient_models file, you have a property change (which should generate a migration you check in -- see 4 below); however, there are also a lot of whitespace changes that add noise to figuring out what's really different in the file. Ideally, you could roll back the whitespace changes and only include the property change.

  3. You can revert all changes to settings.py since you basically never use them -- you opt to use the settings.ini directly (and thanks for stubbing out the public_settings.ini). This isn't ideal, but upon review, I see that it's everywhere and it's a bigger job to get things correct. For now, we can just be consistent, and you should remove any settings you don't use in the settings.py -- which means just reverting it.

  4. There's no migration checked in! I looked at your Slack message, and I think what happened is that the migration you posted was just applying existing migrations. There was no transient_model change. We should Zoom about how to properly test this, but what we'll really need to do is get you a fresh db where the existing changes are already there and you just create and check in a migration that affects the transient_model.

  5. Finally, it would be good to get the single versions for your packages. This might be a slightly more complicated image for us to build if we have to install and uninstall packages, so I want to see if I can just build things with a flat list of single-version dependencies.

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.

3 participants