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

1869 easily accessible dependencies #1903

Merged
merged 3 commits into from
Nov 4, 2021

Conversation

Caddy-Jones
Copy link
Contributor

@Caddy-Jones Caddy-Jones commented Aug 31, 2021

Hi guys

I’ve done some work on issue #1869, but need advice on the next steps.

This pull request does 2 things,
On start up pip is used to generate a list of locally installed modules, and their version number. This only works for modules hosted on pip, so Matplotlib and bumps, but no sasmodules.
On exit, python saves a dictionary of modules used while running this session of SasView as the key, with the accompanying version number as the value. This works for all modules*, including sasmodules.

There are a few areas I need clarification on.

  1. I am currently saving the outputs to 2 .txt files in a new folder called temp. This is a temporary measure, as I am unsure where best to save the outputs. The original issue mentioned saving the versions to the about box or some config file.
  2. Currently pip gathers the installed modules each time SasView is run. Since we cannot guarantee the user has internet access when running SasView, and the installed modules versions are unlikely to change, I think this should be moved to be part of the installation/ update.
  3. There are modules with version numbers that cannot be accessed, such as PyQt5. There is a commented out section of code which uses pip to try and get the module version, however this is a long process, so I decided to not include it. It could also be worth saving a list of modules where the version number could not be ascertained.

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Sep 27, 2021
@wpotrzebowski
Copy link
Contributor

@rozyczko will provide some hints to @Caddy-Jones

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Sep 28, 2021
@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 12, 2021
@butlerpd
Copy link
Member

Still needing to implement suggestions

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 12, 2021
Output is now in the log file
Module list generator now moved to the help tab.
Added docstrings to the functions
@Caddy-Jones
Copy link
Contributor Author

Hi @butlerpd and @wpotrzebowski , I've implemented the suggestions from the call. I've also replied to the comments left by @andyfaff

@Caddy-Jones Caddy-Jones changed the title 1869 WIP easily accessible dependencies 1869 easily accessible dependencies Oct 28, 2021
@wpotrzebowski wpotrzebowski self-requested a review October 29, 2021 12:50
@butlerpd
Copy link
Member

ready for testing on Win

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

The code looks safe enough but am not comfortable judging beyond that. Functionality does seem to work -- though it is a lot of modules which explains why @Caddy-Jones was worried about dumping the output too often. My question now is the json output which does not seem to be compatible with the json viewers I had due to the use of 'xx' rather than "xx" for strings?
It would be helpful if there were an easy way to cut and paste the output into a tool to examine compared to, for example the required files or yml files?

That said, if this passes a better code review the concern above can be figured out later and turned into another issue. This is already a great step forward.

@Caddy-Jones
Copy link
Contributor Author

Caddy-Jones commented Nov 2, 2021

Hi @butlerpd

The pip list command does output a json file, however the log_installed_modules function converts the json to a python dictionary. As a result the output will not be compatible with a json viewer.

There isn't a critical reason for this change, it was just a stylistic choice to keep the outputs of both log_installed_modules and log_imported_modules the same, however both methods could easily be modified to output jsons if that would make life easier

It could also be an idea to compare the outputs to the yml file, and to make any differences easily visible in the functions outputs. Something like

{
    Modules different from yml file: {module: version, module_2: version_2,  etc...}

    Modules consistent with yml file: {module: version, module_2: version_2,  etc...}

    Modules whose version number could not be found: [module_1, module_2, etc...]
}

But I think this may be best answered in a separate pull request

@butlerpd
Copy link
Member

butlerpd commented Nov 2, 2021

Thanks for the clarification. The comparison might be nice but not sure that works once you make a build as I don't think the build slurps in the yml files? So this could cause a problem when trying to build .. and to some extent duplicates the work of "check_packages.py" so not really necessary when not working with a built version?

Json output might help but then maybe not. I think that requires more thinking so I agree that all this can be put into a future ticket as desired.

I will try to test the new changes (responding to @pkienzle ) later today to make sure it doesn't break anything obvious to me at which point I am ready to approve.

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

Works as before for me (without building an installer) so this can now be merged. I note that the following online json formatter/beautifier can deal with most of the issues and "fix" the "json" and render it in an easy to read format.
https://jsonformatter.curiousconcept.com/

@wpotrzebowski wpotrzebowski merged commit 48d2259 into main Nov 4, 2021
@wpotrzebowski wpotrzebowski deleted the 1869_WIP_easily_accessible_dependencies branch November 4, 2021 08:51
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