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

Documentation and changes to installation/maintenance infrastructure #36

Merged
merged 17 commits into from
Sep 23, 2022

Conversation

arokem
Copy link
Collaborator

@arokem arokem commented Jun 24, 2022

For now, I have only started working on the infrastructure side of things, primarily simplifying the setup file in favor of more information in the setup.cfg and (newly added) pyproject.toml file.

Next:

  • Add sphinx documentation.
  • Add a documentation build workflow.

@arokem arokem changed the title WIP + MAIN: Documentation and changes to installation/maintenance infrastructure Documentation and changes to installation/maintenance infrastructure Jun 24, 2022
@arokem
Copy link
Collaborator Author

arokem commented Jun 24, 2022

This is ready for review. The GitHub Actions workflow is configured so that the website gets updated every time that we push a tag to the repository (e.g., for a release). This means that once this is merged, we won't have a website until we push a tag. I guess we've already been talking about cutting a first release over on #33, so that will give us another reason to do so.

The content of the webpage so far is pretty rudimentary and based primarily on the OHBM abstract. If you are interested in commenting on the content, you can find it here.

If you want to download the content to your machine to see how this gets rendered, you can grab the workflow build artifact here. Unzip the file and open the html index file and you can view the website in your browser. This sphinx theme can be customized a lot!

Copy link

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

LGTM!

I confirm that this theme is great! I love it! we use it for fury.gl and I would like to start using it for DIPY to simplify the current craziness

# -- Project information -----------------------------------------------------

project = 'trx-python'
copyright = '2022, The TRX developers'

Choose a reason for hiding this comment

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

To reduce the maintenance, you can use:
copyright = '2021-{0}, The TRX developers'.format(datetime.now().year)

or f-string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice! Automate everything.

Comment on lines 84 to 86
autoapi_type = 'python'
autoapi_dirs = ['../../trx']
autoapi_ignore = ['*test*', '*version*', 'License']

Choose a reason for hiding this comment

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

what is it? Curiosity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC, if you don't ignore this, you'll get API documentation for your test and version files. I can probably remove the "License" entry, though.

@arokem
Copy link
Collaborator Author

arokem commented Jun 24, 2022

Thanks for taking a look and for the suggestions. Implemented in 7de747e

@arokem
Copy link
Collaborator Author

arokem commented Jul 22, 2022

Hey @frheault : I saw that you're back from your travels. Any thoughts about this? Might be good to merge the infra changes before proceeding with other developments

@frheault
Copy link
Collaborator

@arokem I like it! This should be merged before #33 !

Then I will try to re-upload all data with the N+1 'correction' on Figshare with version 0.1 and then we should try to do a release candidate with the HTML, the N+1 'fix' and a working dataset on Figshare.

PS: 0.1 should be a tag on every trx repo if they work with the data on FigShare. I think that if we update the specification we could switch to 1., then 2. so we can follow along dataset and code together

@arokem
Copy link
Collaborator Author

arokem commented Jul 24, 2022

Sounds good! Looks like I need to rebase this on top of master before it can be merged. Let me do that real quick.

@arokem
Copy link
Collaborator Author

arokem commented Jul 24, 2022

Now rebased on master. Note in particular that I've slimmed down setup.py quite significantly. One thing that I was a little bit averse to is the control of installation through setting of environmental variables. I think that we can configure different installation options in setup.cfg instead.

@arokem
Copy link
Collaborator Author

arokem commented Jul 24, 2022

Looks like there are still some installation issues here. I don't have time to figure this out today, so will have to look at again, maybe maybe maybe later this week.

@arokem
Copy link
Collaborator Author

arokem commented Sep 13, 2022

Sorry for the long gap here. Looks like it's installing and testing fine now.

@frheault : do you have the bandwidth to take another look and maybe merge this?

Of note, when testing, DIPY and FURY are now installed from their release version. Is that going to be a problem? I can try fixing this on a subsequent PR, but would like to get the documentation online quickly, by merging this as is, if that's possible.

setup.py Outdated
setup_requires=['packaging>=19.0'],
install_requires=REQUIRES,
scripts=SCRIPTS)
use_scm_version=True,

Choose a reason for hiding this comment

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

Curiosity: This can not be added to setup.cfg or pyproject.toml ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly. See 04ead56.

However, I believe that we'd still need the setup.py file to flexibly install the scripts using the glob pattern.

@skoudoro
Copy link

I think this PR is important to be merged as soon as possible.

Currently, when you create a new environment, trx-python failed to install because of missing libraries like packaging. Also, it is strange to have a version named 0.0.0.

@frheault, Can you have a closer look on this PR and go ahead to merge it. Thanks for your future feedback

@frheault
Copy link
Collaborator

I have a fresh Ubuntu 22.04 installed and this PR made the installation very easy, LGTM!

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

3 participants