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.

Sign up for GitHub

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

PREOPS-4646: feature- Support addition of opsim data to an archive for use by schedview-prenight #20

Merged
merged 19 commits into from
Jan 16, 2024
Merged

PREOPS-4646: feature- Support addition of opsim data to an archive for use by schedview-prenight #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e2ac8e7
Start of sim archiving
ehneilsen Dec 18, 2023
503ff55
Add opsim archive validation function
ehneilsen Dec 19, 2023
7736e46
Add validation and contents to sim archive tests
ehneilsen Dec 19, 2023
9b56db7
include environment files in opsim archive
ehneilsen Dec 19, 2023
0e95d4f
Add tags and label to sim metadata
ehneilsen Dec 20, 2023
0983f8d
add func for querying sim archive metadata
ehneilsen Dec 20, 2023
8ce01c7
replace printing with logging
ehneilsen Dec 20, 2023
0525e94
follow module conventions
ehneilsen Dec 20, 2023
18dd899
include lsst.resources to support sim_archive
ehneilsen Jan 2, 2024
ef07272
initial cli for sim_archive
ehneilsen Jan 3, 2024
6d8271a
include conda package dependency
ehneilsen Jan 3, 2024
bdfd4fb
Support getting sunset from local date
ehneilsen Jan 3, 2024
f7bdb65
driver to run sim and archive results in one command
ehneilsen Jan 3, 2024
7a50d30
match api if drive_sim to sim_runner
ehneilsen Jan 4, 2024
05c0ae7
improve help for make_sim_archive
ehneilsen Jan 4, 2024
bf9afed
be robust wrt absent rewards in archive
ehneilsen Jan 5, 2024
554b4cc
set nside in DDFs so roi works correctly
ehneilsen Jan 5, 2024
13b1f32
install lsst.resources in github actions
ehneilsen Jan 5, 2024
b05caea
skip test that requires schedview if it is not installed.
ehneilsen Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/build_docs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ jobs:
run: |
mamba install --quiet --file=requirements.txt
mamba install --quiet pip
pip install lsst.resources
Copy link
Member

Choose a reason for hiding this comment

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

We should get this into conda-forge ...
but I'm wondering if we can't add it to the requirements.txt file anyway?
If it's not necessarily for most uses, but only for the ones we're in control of for now, this is probably fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, requirments.txt is just a simple text list of packages, and I don't think this format supports pip/pypi packages. Such packages are supported by the conda's yaml format for specifying environments, and perhaps we should move to that format, but maybe that should be done as a separate issue (and corresponding PR).

pip install "documenteer[guide]"

- name: install rubin_scheduler
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/test_and_build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ jobs:
run: |
mamba install --quiet --file=requirements.txt
mamba install --quiet --file=test-requirements.txt
pip install lsst.resources

- name: install rubin_scheduler
shell: bash -l {0}
Expand Down
2 changes: 2 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ dependencies = [
"requests",
"shapely",
"tqdm",
"lsst.resources",
]

[project.optional-dependencies]
Expand All @@ -53,6 +54,7 @@ dev = [
[project.scripts]
scheduler_download_data = "rubin_scheduler.data.scheduler_download_data:scheduler_download_data"
rs_download_sky = "rubin_scheduler.data.rs_download_sky:rs_download_sky"
archive_sim = "rubin_scheduler.sim_archive:make_sim_archive_cli"


[tool.setuptools.dynamic]
Expand Down
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ h5py
requests
shapely
tqdm
conda
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why we need conda as a requirement in a file that is installed with conda?(I'm not following)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One of the bits of metadata that can be included in the archive is the specification for the conda environment that ran the simulation. To get the envirenment, the python code uses the python api to conda, and so the conda package proper must be present.

Copy link
Member

Choose a reason for hiding this comment

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

You mean you are in a situation where this code is not running in a conda environment but needs to get information from a conda environment elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the code is running in a conda environment, and we want to record the details of that environment as metadata along with the results of a simulation run using code in that environment, to make it easy to answer questions like "What version of X was being using in the process that produced output Y?"

Copy link
Member

Choose a reason for hiding this comment

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

So in that case conda is already installed in your conda env and you don't need pypi to install it. I don't think it should be listed as a formal dependency but should have an import protection around it like we do in lsst-utils package when retrieving conda package versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's in the conda base environment, not the environment that is actually running the code and writing it to the archive. For example:

(schedview) [neilsen@sdfrome002 rubin_scheduler]$ python
Python 3.11.6 | packaged by conda-forge | (main, Oct  3 2023, 10:40:35) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from conda.cli.main_list import print_packages
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'conda'

Copy link
Member

Choose a reason for hiding this comment

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

Yes. That's why we add conda to rubin-env. Does adding it to the formal dependencies cause problems when pip installing?

Copy link
Member

Choose a reason for hiding this comment

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

Since we have a pyproject.toml file with project dependencies, I think you shouldn't have a problem with pip installation. I guess if conda is part of Rubin-env, it's reasonable to add as a dependency for this package.

4 changes: 2 additions & 2 deletions rubin_scheduler/scheduler/example/example_scheduler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,10 +1087,10 @@
euclid_obs = np.where((obs_array["note"] == "DD:EDFS_b") | (obs_array["note"] == "DD:EDFS_a"))[0]
all_other = np.where((obs_array["note"] != "DD:EDFS_b") & (obs_array["note"] != "DD:EDFS_a"))[0]

survey1 = ScriptedSurvey([bf.AvoidDirectWind(nside=nside)], detailers=detailers)
survey1 = ScriptedSurvey([bf.AvoidDirectWind(nside=nside)], nside=nside, detailers=detailers)

Check warning on line 1090 in rubin_scheduler/scheduler/example/example_scheduler.py

View check run for this annotation

Codecov / codecov/patch

rubin_scheduler/scheduler/example/example_scheduler.py#L1090 

Added line #L1090 was not covered by tests
survey1.set_script(obs_array[all_other])

survey2 = ScriptedSurvey([bf.AvoidDirectWind(nside=nside)], detailers=euclid_detailers)
survey2 = ScriptedSurvey([bf.AvoidDirectWind(nside=nside)], nside=nside, detailers=euclid_detailers)

Check warning on line 1093 in rubin_scheduler/scheduler/example/example_scheduler.py

View check run for this annotation

Codecov / codecov/patch

rubin_scheduler/scheduler/example/example_scheduler.py#L1093 

Added line #L1093 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

The addition of nside here -- this seems outside the range of the "add archiving to the simulation outputs" ... was this a case of something that went missing from the example_scheduler?
(I ask because I think we're in a little bit of a race condition between example scheduler in rubin_scheduler and example scheduler in the sims_featureScheduler_runs repos, and we need to communicate super clearly about changes that might be needed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One of the very recent changes to rubin_scheduler (which added the direct wind basis functions to scripted surveys) caused schedview's tests to fail because the nside of the survey in the example scheduler in the didn't match the nside of its basis functions. (The schedview unit tests set a smaller nside to make the test go faster.)
So, this fix was needed to make schedview's tests pass after the rebase before the pull request, although really it's a fix for a bug in the pull that got applied in the rebase.

Copy link
Member

Choose a reason for hiding this comment

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

I pulled that change over to sims_featureScheduler_runs3.4. Also added it to survey1 above since it looks like that should need it too.

survey2.set_script(obs_array[euclid_obs])

return [survey1, survey2]
Expand Down
1 change: 1 addition & 0 deletions rubin_scheduler/sim_archive/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from .sim_archive import *

Check warning on line 1 in rubin_scheduler/sim_archive/__init__.py

View check run for this annotation

Codecov / codecov/patch

rubin_scheduler/sim_archive/__init__.py#L1 

Added line #L1 was not covered by tests
Loading
Loading