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

DM-39205: Add READMEs for ingredients and pipelines dirs #65

Merged
merged 2 commits into from May 16, 2023

Conversation

leeskelvin
Copy link
Contributor

No description provided.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

Looks good, Lee! Only minor comment would be for you to consider whether you want to give a concrete example that can be copied from the README and run, or whether you want to have a generic PATH/TO/MY/PIPELINE path here.

@leeskelvin
Copy link
Contributor Author

Happy to put in a concrete pipeline YAML if you think it would improve the legibility here. My concern with doing so is that it may go out of sync with future stack versions if the example we choose is moved, but keen to hear your thoughts. I also don't know the best example to choose, so recommendations welcome.

@arunkannawadi
Copy link
Member

I thought about the possibility of it going out of sync, but by phrasing it as an example, we could perhaps hope that the reader would generalize it to a different path. The example I'd go for is [VisualizeVisit.yaml](https://github.com/lsst/drp_pipe/blob/main/pipelines/VisualizeVisit.yaml) inside of pipelines. The README could say that in order to visualize the VisualizeVisit.yaml, run pipeline build .... That way, even if VisualizeVist.yaml moves elsewhere, it's evident how to run the command, although the advantage of simply copying might not exist then.

@leeskelvin
Copy link
Contributor Author

leeskelvin commented May 16, 2023

My only concern with VisualizeVisit is that the YAML is very basic. There are no imports, and if a user just looks at the YAML directly they may just ask "why do I need to run pipetask build, when I can just see everything here on GitHub?"

If you really do want a specific pipeline YAML here in this example, my feeling is that we need something a little more complex (i.e., containing imports) but not too complex. RC2_SUBSET would be nice as it's a tutorial pipeline. However, that pipeline suffers from the fact that it's the only pipeline that can't be built in its entirety without raising an error (two subsets output the same dataset type), which may lead to some additional confusion.

If I had to choose, I'd perhaps go for pipelines/LATISS/DRP.yaml#step1. It's a relatively simple pipeline, not too many input dataset types (an important consideration if users are building pipeline graph PDFs for visualization purposes), and is likely to remain in this location for the foreseeable future.

@arunkannawadi
Copy link
Member

Your suggestion seems fine to me!

@arunkannawadi
Copy link
Member

Also, feel free to completely ignore this and go ahead with as it is currently.

@arunkannawadi
Copy link
Member

While you are at it, could you add a commit that updates the COPYRIGHT year to say 2021-2023 please?

@leeskelvin leeskelvin merged commit ef03c79 into main May 16, 2023
2 checks passed
@leeskelvin leeskelvin deleted the tickets/DM-39205 branch May 16, 2023 15:29
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.

None yet

2 participants