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

Second attempt for easily accessible dependencies #1961

Conversation

wpotrzebowski
Copy link
Contributor

Follow up on #1903 that broke build and was reverted

@wpotrzebowski
Copy link
Contributor Author

ready for testing on OSX

@wpotrzebowski
Copy link
Contributor Author

@Caddy-Jones: can you please take a look on these warnings apparently causing Jenkins build failure. Note that you need to checkout new branch becuse of (re)reverting...

/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer:4: WARNING: Unexpected section title.

Methods
-------
/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.format_unattainable_modules_list:10: WARNING: Unexpected section title.

Parameters
----------
/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.format_unattainable_modules_list:17: WARNING: Unexpected section title.

Returns
-------
/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.get_imported_modules:8: WARNING: Unexpected section title.

Returns
-------
/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.log_imported_modules:7: WARNING: Unexpected section title.

Returns
-------
/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.log_installed_modules:7: WARNING: Unexpected section title.

Returns
-------
/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.remove_duplicate_modules:10: WARNING: Unexpected section title.

Parameters
----------
/Users/sasview/Jenkins/workspace/SasView-OSX10.13-ghprb/sasview/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.remove_duplicate_modules:15: WARNING: Unexpected section title.

module_versions_dict = {'python': sys.version, 'SasView': sas.sasview.__version__}
unattainable_modules = []
# Generate a list of standard modules by looking at the local python library
standard_lib = [path.stem.split('.')[0] for path in pathlib.Path(pathlib.__file__).parent.absolute().glob('*')]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can combine the standard lib packages, the builtin packages and sas into a single set. This simplifies the test in the loop:

standard_path = pathlib.Path(pathlib.__file__).parent.absolute()
standard_lib = {path.stem.split('.')[0] for path in standard_path.glob('*')} 
standard_lib.update(sys.builtin_module_names)
standard_lib.add('sas') # include 'sas' as standard lib

Note that this will not do what you want in the shipped application because pathlib may be in a zip file instead of on the disk. It may even fail in the application since pathlib.__file__ might not be defined. This is why I suggested hard-coding the list rather than building it on the fly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, so I've included your standard_path code in a try except block, with a hard coded list of bultin modules in the event that pathlib.file causes an error.

Could someone check if pathlib.file does indedd fail in the shipped application, as I would not know how to check.

If it does fail, then the list of builtin modules will probably have to be checked each time the python version SasView uses is updated.

May be worth having this list in an external file, making the job of updating the list easier?

installed_modules = {'python': sys.version}

# Get python modules installed locally
installed_modules_json = json.loads(subprocess.check_output("pip list -l --format=json"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Fails for installed application.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pip fails?

@Caddy-Jones
Copy link
Contributor

Hi @wpotrzebowski, The errors appear to come from the titles I used in the docstrings, causing “WARNING: Unexpected section title.” I used the NumPy/SciPy Docstring format for the docstings in the PackageGatherer class. Is there a different docstring standard used by SasView?

@pkienzle
Copy link
Contributor

pkienzle commented Nov 5, 2021

Hi @wpotrzebowski, The errors appear to come from the titles I used in the docstrings, causing “WARNING: Unexpected section title.” I used the NumPy/SciPy Docstring format for the docstings in the PackageGatherer class. Is there a different docstring standard used by SasView?

Coding standards for docstrings are here. We don't suggest (or follow) any particular style and there is wide variation across the project. So long as it works with sphinx it's okay.

Instead of underlined Section heading I've used **Section heading** to get a similar effect.

@Caddy-Jones
Copy link
Contributor

Caddy-Jones commented Nov 8, 2021

Is there any way to add a new branch to this pull request for the code changes? If not i can set up a new PR

@wpotrzebowski
Copy link
Contributor Author

@Caddy-Jones I think the easiest is to checkout this branch revert-1958-revert-1903-1869_WIP_easily_accessible_dependencie and add code there.

logging.error(f"{x} when attempting to access {mod} version using .version")
pass
else:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above re: exception handling.

And to repeat, this will probably not do what you want.

Example

>>> h5py.version
<module 'h5py.version' from '/Users/pkienzle/conda/envs/py38/lib/python3.8/site-packages/h5py/version.py'>

Though in this case h5py.__version__ exists so you won't get here.

If you insist on keeping this clause at least check if isinstance(package.version, str).

@Caddy-Jones
Copy link
Contributor

Hi @pkienzle .

Thanks for your feedback, the reason i included the .version check is because some modules, do not have a version attribute, and instead .version returns the version number. But i will include a check to see if the version number is a string or a float.

@butlerpd butlerpd changed the base branch from main to release_5.0.5 November 20, 2021 22:38
@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 22, 2021
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 23, 2021
@wpotrzebowski
Copy link
Contributor Author

@wpotrzebowski will take a look on it

@wpotrzebowski
Copy link
Contributor Author

ready for testing on OSX

@wpotrzebowski
Copy link
Contributor Author

@Caddy-Jones you may consider fixing this one before we merge:
/src/sas/qtgui/MainWindow/PackageGatherer.py:docstring of sas.qtgui.MainWindow.PackageGatherer.PackageGatherer.remove_duplicate_modules:15: WARNING: Field list ends without a blank line; unexpected unindent

@Caddy-Jones
Copy link
Contributor

Hi @wpotrzebowski , is there a way to test that the docstring is formated correctly?

@wpotrzebowski
Copy link
Contributor Author

Hi @wpotrzebowski , is there a way to test that the docstring is formated correctly?

@Caddy-Jones I've just tested it by running docs build locally and looking for warnings (you can '-w' oprtion to build_sphinx.py to output warnings to file).

@Caddy-Jones
Copy link
Contributor

Hi @wpotrzebowski , where am i supposed to run build_sphinx.py from, i keep getting errors with the script unable to find sasmodels.

@smk78
Copy link
Contributor

smk78 commented Nov 24, 2021

@Caddy-Jones you should be able to do python setup.py build docs from the sasview top level. Otherwise, look in \docs\sphinx-docs. But you have to build the sasmodels docs first. Go to sasmodels\doc and make clean html.

@Caddy-Jones
Copy link
Contributor

Hi guys, I just cannot recreate the results @wpotrzebowski has gotten, can you give me the command line input you used please

@smk78
Copy link
Contributor

smk78 commented Nov 25, 2021

@Caddy-Jones I've pulled your branch and built the docs locally and I'm seeing the exact same doc error that @wpotrzebowski reports. I'm running Sphinx 1.8.4 (according to pip list). But nothing I've tried so far has fixed the error! :-(

@Caddy-Jones
Copy link
Contributor

Apologies but I can’t seem to generate the sphinx documentation, anything I try gives me an error. Could somebody give me a step by step explanation of how to generate the documents, or point me to the location of such a thing. For example trying to do any make command gives me
'make' is not recognized as an internal or external command,operable program or batch file.
Trying to run the sphinx-build.py script gives me

=== Sasmodels Docs ===
Traceback (most recent call last):
  File "build_sphinx.py", line 294, in <module>
    rebuild()
  File "build_sphinx.py", line 280, in rebuild
    retrieve_user_docs()
  File "build_sphinx.py", line 134, in retrieve_user_docs
    copy_tree(SASMODELS_MODEL_SOURCE, SASMODELS_MODEL_TARGET)
  File "C:\Code_base\Anaconda3\envs\qt5_win\lib\distutils\dir_util.py", line 124, in copy_tree
    "cannot copy tree '%s': not a directory" % src)
distutils.errors.DistutilsFileError: cannot copy tree 'C:\Local_repo\sasview\docs\sphinx-docs\..\..\..\sasmodels\doc\model': not a directory

setup.py build docs Isn’t an option for sasmodels, and doesn’t give me the results wojeck had for sasview, instead i get an error:

copying src\sas\sasview\test\upcoming_formats\1000A_sphere_sm.xml -> build\lib\sas\sasview\test\upcoming_formats
running docs
========= check for sasmodels at C:\Local_repo\sasmodels\doc ============
============= Building sasmodels model documentation ===============
error: [WinError 2] The system cannot find the file specified

@smk78
Copy link
Contributor

smk78 commented Nov 25, 2021

@Caddy-Jones : Go to your sasmodels repo. cd doc. make clean html. If make gives you an error then presumably you don't have it installed? Once the sasmodels docs are built, cd to your sasview branch and python setup.py build docs. The sasview doc build copies the built sasmodels docs to the sasview\docs\sphinx-docs\build folder. If all has worked you should then be able to cd \html and open index.html and navigate around the docs. If you're using an Anaconda environment, remember you have to activate the environment before doing any of this! :-)

@Caddy-Jones
Copy link
Contributor

Thanks @smk78 , turns out make wasn't installed and there was an issue with my conda environment which prevented me from installing it. I've fixed the issue with the docstring so there hopefully should be no further issues

@smk78
Copy link
Contributor

smk78 commented Nov 25, 2021

Yes, I've just pulled your branch and re-made the docs and they pass compilation now. Good!

@wpotrzebowski
Copy link
Contributor Author

ready for testing on OSX

@wpotrzebowski wpotrzebowski merged commit afc53ff into release_5.0.5 Nov 28, 2021
@wpotrzebowski wpotrzebowski deleted the revert-1958-revert-1903-1869_WIP_easily_accessible_dependencies branch November 28, 2021 21:14
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.

5 participants