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 a Contributor Guide #42

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaladh-singhal
Copy link
Member

Aims to fix #17 esp the bulk usage part

I've tried to draft a documentation based on my development experience on this repository so far. It's very minimal and can use a lot of changes (and maybe restructuring too?)

Marking it as draft since it needs feedback from other developers (@bsipocz, @troyraen, ...?) before adding more content


## Environment setup and workflow
- Clone the repo
- Create a fresh python environemt with `.binder/requirements.txt` file
Copy link
Member Author

Choose a reason for hiding this comment

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

@bsipocz I noticed you recently moved requirements.txt to .binder/ from tutorials/. Can we keep it in tutorials/ and create a symlink to it in .binder/?

Copy link
Member

Choose a reason for hiding this comment

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

Symlinks notoriously won't work on windows, at least I had issues with keeping symlinks in repos before (and we only symlinked licences and contributing guides rather than actually important content)

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought users wouldn't need to touch this requirements.txt file all since notebooks in this repo should always pip install what they need in the first code cell -- I thought this file was only here for the rendering? I do see that the instructions below have some dependency on in it (i.e., assuming jupytext extension is installed). Since my workflow with notebooks is so different I don't know exactly what the user will need, so I'm fine with whatever's easiest (either telling them to install that extension separately or just pointing them to the requirements.txt file even though it isn't intended for them). Although, the other repo doesn't have a requirements.txt, so if this file gets moved there that's another factor to think about.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

I'm not yet sure if this is the right repo for this document, or we should rather keep it in the other repo (after all that's where we have the template, too).

Anyway, I'll give this a more detailed review later this week.

- Create a fresh python environemt with `.binder/requirements.txt` file
- Open `jupyter lab` or `jupyter notebook` (so that jupytext extension can take effect)
- Then right-click on a MyST markdown notebook, choose `Open With` -> `Notebook`
or `Jupytext Notebook` (more info [here](https://nasa-navo.github.io/navo-workshop/00_SETUP.html#handling-notebooks-in-myst-markdown-format)).
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 link more upstream than NAVO, e.g. numpy

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you share the link? Last I checked numpy docs, I got confused because they were instructing to "pairing the notebook and md" but then I learned from you that that's not required.

Copy link
Member

Choose a reason for hiding this comment

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

I knew it would bite me back, we need to cleanup the numpy docs first, 😅 : numpy/numpy-tutorials#186

Copy link
Contributor

@troyraen troyraen left a comment

Choose a reason for hiding this comment

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

Thanks @jaladh-singhal, this is helpful. I don't often work in a jupyter environment (I'm much more likely to convert the .md to .py and just use an editor), so I don't really have thoughts/opinions about the actual instructions here except to say that I think they're much needed since contributors are likely to be working this way.

Organizationally, we should merge this with the template @bsipocz referred to so that we have one consistent set of "contributing" documentation. That template is for new notebooks and is here: https://github.com/IPAC-SW/ipac-sp-notebooks/tree/main/template.

In terms of which repo, we had talked about having contributions go into the other repo both so that they can be worked on in private and so we can better control the content in this repo (since the rendering, etc. means there are more requirements/constraints here). But I'll defer to @bsipocz on that since she does most of the management tasks that would be affected.

In terms of the content, there's a little bit of overlap so we should make sure those parts are consistent (and this will depend on which repo we choose). Also, maybe the content in the template's README should actually go in the 'new notebook' section of this file. Then we wouldn't need that separate file. @jaladh-singhal take a look at that readme and the template and see what you think.


## Environment setup and workflow
- Clone the repo
- Create a fresh python environemt with `.binder/requirements.txt` file
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought users wouldn't need to touch this requirements.txt file all since notebooks in this repo should always pip install what they need in the first code cell -- I thought this file was only here for the rendering? I do see that the instructions below have some dependency on in it (i.e., assuming jupytext extension is installed). Since my workflow with notebooks is so different I don't know exactly what the user will need, so I'm fine with whatever's easiest (either telling them to install that extension separately or just pointing them to the requirements.txt file even though it isn't intended for them). Although, the other repo doesn't have a requirements.txt, so if this file gets moved there that's another factor to think about.

Comment on lines +30 to +31
```python
jupytext --to md:myst notebook.ipynb
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```python
jupytext --to md:myst notebook.ipynb
```sh
jupytext --to md:myst notebook.ipynb

jupytext --to md:myst notebook.ipynb
```

Then add it to `tutorials/` and also add its path in a relevant TOC in `index.md`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just flagging this line because there are similar instructions in the existing docs in the other repo, but the process is different in that repo. So we need to sync them up depending on which repo we chose.

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.

DOC: contributing and usage guides
3 participants