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

Limit dependencies to strictly necessary packages. #383

Merged

Conversation

mkelley
Copy link
Member

@mkelley mkelley commented Jul 28, 2023

Make scipy, ads, astroquery, and synphot optional packages, limiting the required packages to numpy and astropy.

  • Add new functions and decorators in utils to facilitate testing for optional packages, and raising errors when they are needed.
  • Skip tests that require the above packages when they are not available.

To do:

  • Verify that tests are working when the above packages are installed (i.e., alldeps).
  • Update documentation so that it can be built without optional packages?

Addresses #382

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (ca24266) 76.84% compared to head (e342b89) 77.52%.

Files Patch % Lines
sbpy/data/tests/test_orbit_remote.py 20.00% 4 Missing ⚠️
sbpy/activity/gas/tests/test_prodrate_remote.py 25.00% 3 Missing ⚠️
sbpy/calib/tests/test_sun.py 80.00% 3 Missing ⚠️
sbpy/calib/tests/test_core.py 98.13% 2 Missing ⚠️
sbpy/photometry/tests/test_iau.py 75.00% 2 Missing ⚠️
sbpy/activity/gas/productionrate.py 93.33% 1 Missing ⚠️
sbpy/data/tests/test_ephem_remote.py 50.00% 1 Missing ⚠️
sbpy/utils/tests/test_decorators.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
+ Coverage   76.84%   77.52%   +0.67%     
==========================================
  Files          78       81       +3     
  Lines        6974     7096     +122     
==========================================
+ Hits         5359     5501     +142     
+ Misses       1615     1595      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Show resolved Hide resolved
sbpy/activity/gas/productionrate.py Outdated Show resolved Hide resolved
sbpy/activity/gas/tests/test_data.py Outdated Show resolved Hide resolved
sbpy/bib/core.py Outdated Show resolved Hide resolved
sbpy/bib/core.py Outdated Show resolved Hide resolved
sbpy/utils/tests/test_decorators.py Outdated Show resolved Hide resolved
docs/install.rst Outdated Show resolved Hide resolved
@mkelley
Copy link
Member Author

mkelley commented Aug 24, 2023

Cannot build the docs. Sphinx crashes with the traceback:

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/cmd/build.py", line 298, in build_main
    app.build(args.force_all, args.filenames)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/application.py", line 355, in build
    self.builder.build_update()
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 293, in build_update
    self.build(to_build,
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 313, in build
    updated_docnames = set(self.read())
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 420, in read
    self._read_serial(docnames)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 441, in _read_serial
    self.read_doc(docname)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 498, in read_doc
    publisher.publish()
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/core.py", line 234, in publish
    self.document = self.reader.read(self.source, self.parser,
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/io.py", line 105, in read
    self.parse()
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/readers/__init__.py", line 76, in parse
    self.parser.parse(self.input, document)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/sphinx/parsers.py", line 81, in parse
    self.statemachine.run(inputlines, document, inliner=self.inliner)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 169, in run
    results = StateMachineWS.run(self, input_lines, input_offset,
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/statemachine.py", line 233, in run
    context, next_state, result = self.check_line(
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/statemachine.py", line 445, in check_line
    return method(match, context, next_state)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 2785, in underline
    self.section(title, source, style, lineno - 1, messages)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 325, in section
    self.new_subsection(title, lineno, messages)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 391, in new_subsection
    newabsoffset = self.nested_parse(
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 279, in nested_parse
    state_machine.run(block, input_offset, memo=self.memo,
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 195, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/statemachine.py", line 233, in run
    context, next_state, result = self.check_line(
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/statemachine.py", line 445, in check_line
    return method(match, context, next_state)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 2785, in underline
    self.section(title, source, style, lineno - 1, messages)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 325, in section
    self.new_subsection(title, lineno, messages)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 391, in new_subsection
    newabsoffset = self.nested_parse(
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 279, in nested_parse
    state_machine.run(block, input_offset, memo=self.memo,
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 195, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/statemachine.py", line 233, in run
    context, next_state, result = self.check_line(
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/statemachine.py", line 445, in check_line
    return method(match, context, next_state)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 2355, in explicit_markup
    nodelist, blank_finish = self.explicit_construct(match)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 2367, in explicit_construct
    return method(self, expmatch)
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 2104, in directive
    return self.run_directive(
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/parsers/rst/states.py", line 2154, in run_directive
    result = directive_instance.run()
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/pytest_doctestplus/sphinx/doctestplus.py", line 23, in run
    if re.match('win32', self.content[0]):
  File "/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/383/lib/python3.10/site-packages/docutils/statemachine.py", line 1136, in __getitem__
    return self.data[i]
IndexError: list index out of range

which doesn't mean anything to me. I can build ffa5115, so it is definitely something I changed with sbpy. I will look into this some more. Meanwhile, the rest of the checks are good.

@mkelley
Copy link
Member Author

mkelley commented Aug 26, 2023

Resolving the documentation build error is pending some more information on the problem via scientific-python/pytest-doctestplus#219

@mkelley
Copy link
Member Author

mkelley commented Aug 28, 2023

@jianyangli Remote tests are also passing locally with and without alldeps. Ready for a re-review.

@mkelley mkelley requested a review from hhsieh00 August 31, 2023 18:11
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a general question about how to set up the test in the case when the functions being called inside a function depend on an optional package.

For example, function a doesn't directly uses synphot, but a calls function b, and b uses synphot. Should we leave everything (check the availability of a module, test) about optional packages to the functions that directly use the module, and don't do this check for higher level functions or classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess just consider the user experience for each? But also consider what happens if function b is later revised and no longer needs synphot. I guess then the requirement on a will be obsolete and might be difficult to find. So, best to leave it to the point the package is actually used?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

@mkelley mkelley removed the request for review from hhsieh00 November 30, 2023 17:25
Copy link
Contributor

@jianyangli jianyangli left a comment

Choose a reason for hiding this comment

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

I don't see any obvious problems or questions now. I think it's ready to deploy.

@mkelley
Copy link
Member Author

mkelley commented Jan 10, 2024

Thanks, @jianyangli !

@mkelley mkelley merged commit 1315915 into NASA-Planetary-Science:main Jan 10, 2024
9 checks passed
@mkelley mkelley deleted the optional-dependencies-2023.07 branch January 10, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants