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
DM-11325: Make prototype pipeline eups-compatible #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the package dependencies in ups/
are correct, so please change those before committing. A few minor comments elsewhere.
ingested/* | ||
calibingested/* | ||
data/* | ||
MasterCals/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vestigial from before we had ap_verify_hits2015
, I assume?
doc/conf.py
Outdated
_g = globals() | ||
_g.update(build_package_configs( | ||
project_name="afw", | ||
copyright="2017 Association of Univerities for " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Three afw
references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did a dumb substitution of afw
-> ap_verify
/ap.verify
on my end and it doesn't seem to have broken documentation building...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
ups/ap_pipe.table
Outdated
|
||
setupRequired(obs_decam) | ||
setupOptional(ap_verify_hits2015) | ||
setupOptional(ap_verify) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely shouldn't have ap_verify
here, because that gives a circular dependency. I don't think ap_verify_hits2015
is appropriate either.
ups/ap_pipe.cfg
Outdated
dependencies = { | ||
"required": ["utils", "ndarray"], | ||
"buildRequired": ["boost_test", "pybind11", "ndarray", "pex_exceptions", "obs_decam"], | ||
"optional": ["ap_verify_hits2015", "ap_verify"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think ap_pipe.cfg
needs to contain many packages, particularly not Python-only ones, because it's used by Scons to set up paths and the like. But I'll admit I don't know for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw yours was empty for dependencies
; I went with the one in the template repo and just added obs_decam
. But I suspect you're right about not needing to have anything in here for python-only dependencies, so I'll make it empty instead.
a3b7526
to
a0e9e40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
No description provided.