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

Adding OpenMDAO Sphinx Extensions #11

Merged
merged 9 commits into from
Jun 6, 2022
Merged

Adding OpenMDAO Sphinx Extensions #11

merged 9 commits into from
Jun 6, 2022

Conversation

lamkina
Copy link
Contributor

@lamkina lamkina commented Jun 5, 2022

Purpose

This PR ports over sphinx extensions from OpenMDAO 3.9 that are unsupported in OM 3.10 or greater. Also included is an import sorting configuration file consistent with pyOptSparse. The setup.py file was also modified to include the necessary dependencies for the extensions.

Expected time until merged

This PR should take a week or less to merge. As these extensions are difficult to test, it may require another PR to fix bugs that arise once these extensions are used in other MDO Lab documentation.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@ewu63 ewu63 self-requested a review June 5, 2022 19:34
Copy link
Collaborator

@ewu63 ewu63 left a comment

Choose a reason for hiding this comment

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

A few high-level questions:

  • Why did you add the isort config? Is it used by anything here?
  • I'd like to avoid adding numpy as a dependency if possible, since it's pretty big. Do you think the context manager printoptions will be used by people?
  • I don't think the empty init file is necessary

Everything else is fine. Once we have this up, we can use OAS or something to test these extensions, same way we are currently testing extensions via pyhyp.

@lamkina
Copy link
Contributor Author

lamkina commented Jun 5, 2022

  • I added isort just to make things look nice.
  • The numpy printoptions call is used in the run_code method of the docutil.py file. That run_code method is used in the embed_code extension. It appears it's mainly used to get more precision when printing floats in numpy arrays. I can look into removing this dependency.
  • I thought we would want the exts folder to be treated as a module. I can remove the init file if it's not necessary.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 5, 2022

  • I added isort just to make things look nice.

Given the number of repositories we have, I think it would be better if we do not have separate config files in each repo (unless they are different for some reason). Instead, we should just have a single file (kept in the .github repo) which is shared for everything. On my computer, I've defined an alias for isort that would invoke that specific config file. This ensures consistency across our organization and makes things more manageable. As for pyOptSparse, I can just remove that file.

  • The numpy printoptions call is used in the run_code method of the docutil.py file. That run_code method is used in the embed_code extension. It appears it's mainly used to get more precision when printing floats in numpy arrays. I can look into removing this dependency.

Yeah I see, in that case we can keep this unless you see an easy way to remove it. It seems like it could be useful in situations.

  • I thought we would want the exts folder to be treated as a module. I can remove the init file if it's not necessary.

I don't think you need it to import stuff anymore in py3, and in any case I don't see a use case for people directly importing those extensions.

@lamkina
Copy link
Contributor Author

lamkina commented Jun 6, 2022

I removed the __init__.py files but now the imports are broken. I think I need them because the current setup of the repo requires absolute imports. Maybe I'm wrong, is there a different way to do this?

I also got rid of the isort file in favor of linking to the mdolab/.github config file. I will look into the numpy dependency today, but I don't see an easy way to remove it as of now.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 6, 2022

Huh that's really weird, how has it been working up till now? The error is not coming from any of the new additions. I don't have time to look into this further, so if this passes the tests I'm okay with it.

@lamkina
Copy link
Contributor Author

lamkina commented Jun 6, 2022

It worked before this PR because the existing extensions didn't depend on any other sub-modules within the main package. I added separate utils and ext modules and I import stuff from the utils module into the exts module. I thought this made the most sense because there may be instances where we use stuff from utils and ext separately.

@lamkina
Copy link
Contributor Author

lamkina commented Jun 6, 2022

I think it's ready to merge if you agree with my reasoning.

Copy link
Collaborator

@ewu63 ewu63 left a comment

Choose a reason for hiding this comment

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

LGTM

@ewu63 ewu63 merged commit 7b26238 into mdolab:main Jun 6, 2022
@lamkina lamkina deleted the om_ext branch June 6, 2022 19:13
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