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

Add initial porting guidelines #95

Merged
merged 16 commits into from
Sep 27, 2024
Merged

Add initial porting guidelines #95

merged 16 commits into from
Sep 27, 2024

Conversation

jashapiro
Copy link
Member

Closes #71

This is the initial draft of the porting guidelines for add modules from OpenScPCA-analysis to OpenScPCA-nf. I tried to cover everything I had thought of in #71 and then some, but I am sure that what I have here is still incomplete!

I also made a few changes to the workflow itself:

  • adding emit: statements to a couple workflows which I did mostly to test syntax (they have to be at the end, which is annoying, since that isn't the case for process outputs)
  • removing a module-specific parameter (a container name) because apparently that is deprecated behavior (I have a question in the docs related to this)

I think the goal at this stage is to make sure we have most of the overall structure set, both from the POV of adding a modules and the structure of this document. I'm more confident in the former than the latter, though see some caveats below. I wrote the sections in kind of disconnected chunks (no IA!) , so it may be a bit choppy. Which is to say I am open to some significant restructuring!

Caveats:

  • I assume a certain amount of familiarity with Nextflow, which I think is unavoidable, but I am happy to add more detail where it might be helpful. I also expect to add links to some relevant Nextflow docs, but I have not done that yet.
  • I don't yet have instructions about using an emitted value as an input to another workflow; I think I wanted to wait until we had an example of that to really test it out

Copy link
Member

@sjspielman sjspielman left a comment

Choose a reason for hiding this comment

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

Overall this seems like a good place to start from. I am sure as you are that content is missing, but there's so much here that I think it's a good baseline and we'll find out what is missing as more modules get ported...! The main missing items I noticed were (and these, certainly the 2nd, might be different PRs):

  • I think we'd benefit from a section that describes the main workflow and its relationship to module more, including adding a module to the main workflow.
  • This probably does not have a good one-size-fits-all answer, but - how to actually run the workflow, e.g. as part of development.

Most of my suggestions focus on spacing and organization within the existing structure, but I think the overall order of topics here is fine at least for now.

porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Show resolved Hide resolved
porting-modules.md Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
@jashapiro
Copy link
Member Author

I did some reorganization and made updates and refinements that will hopefully address most of @sjspielman's comments.

I added some comments about the main workflow (referred to here as the "default" workflow to distinguish it from the other entrypoints that do exist in the root main.nf file).
Container definitions are now moved to a central location.

I think I would leave the questions about running and testing to a future update, perhaps in concert with the next module being added. I have some thoughts, but I also want to take some more time to think about what to recommend and then we will have to test that those instructions are correct!

Copy link
Member

@sjspielman sjspielman left a comment

Choose a reason for hiding this comment

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

This is looking good! Content wise I don't have much to say at this point, other than for items which we plan to circle back to (like how to run/test during development), let's at least open a (small for now!) reminder issue to indeed circle back. The main reason I'm not approving now is b/c of the broken anchor link.

Another general comment I have is that I think we should do a round of copy editing here to update some passive voice -> active voice, following guidelines we used for our docs and use more "you" to refer to the readers here.

For example, I might change this as follows:

# current sentence
Instead, the files that will be required for each sample should be selected using the `Utils.getLibraryFiles()` function or similar methods.

# you-ified
Instead, you should use the `Utils.getLibraryFiles()` function or similar methods to select files required for each sample.

I do think it's eminently reasonable for that to be a separate PR though, possibly bundled with other style decisions?

porting-modules.md Outdated Show resolved Hide resolved
Comment on lines 60 to 61
Each module directory should contain a `readme.md` file with the following contents:
that provides a brief description of the module and its purpose, as well as a link to the original module in `OpenScPCA-analysis`.
Copy link
Member

Choose a reason for hiding this comment

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

looks like this was in the process of being updated but wasn't finished?

porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Outdated Show resolved Hide resolved
porting-modules.md Show resolved Hide resolved
@jashapiro
Copy link
Member Author

I made things much more (but not completely) active, and finished up the readme bullets that had escaped me earlier.

I didn't use a lot of "you should" but instead made things more direct commandments, in a lot of cases. I hope that is okay.

Copy link
Member

@sjspielman sjspielman left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@jashapiro jashapiro merged commit 1cb4171 into main Sep 27, 2024
2 checks passed
@jashapiro jashapiro deleted the jashapiro/porting-guide branch September 27, 2024 13:26
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.

Create porting guidelines document
2 participants