Fix issue with path when mvs is installed as a package by Bachibouzouk · Pull Request #607 · rl-institut/multi-vector-simulator

Bachibouzouk · 2020-10-14T09:42:37Z

Changes proposed in this pull request:

Remove REPORT_PATH constant
Add report assets and example simulation to package_data in setup.py
Add a util function to copy report asset to simulation output folder when user generates the report
Remove reference to git branch ID in the report
Make sure report can be generated even if figures are missing from simulation outputs
Move the code located in mvs_report.py into multi_vector_simulator.cli:report
Add entrypoints for mvs_tool and mvs_report in ´setup.py´
Update installation steps in README and in RTD
If default folder does not exist when code is executed, example simulation's inputs are used from package_data

To be able to use package_data, one has to install the package locally python setup.py install, for developers python setup.py develop or pip install -e .. However this does not install package_data properly, so developers cannot use complete default values. They can use ´mvs_tool -i tests/inputs´ when being in the root of the repository (maybe need to add this in CONTRIBUTING, but after we merge #601)

The following steps were realized, as well (if applies):

Use in-line comments to explain your code
Write docstrings to your code (example docstring)
For new functionalities: Explain in readthedocs
Write test(s) for your new patch of code (pytests, assertion debug messages)
Update the CHANGELOG.md
Apply black (black . --exclude docs/)
Check if benchmark tests pass locally (EXECUTE_TESTS_ON=master pytest)

_{For more information on how to contribute check the CONTRIBUTING.md.}

Bachibouzouk · 2020-10-14T20:47:37Z

@smartie2076 - it could have been split in two PRs, but some features were depending on one another so I grouped everything under one PR

The eland logo and styles.css files are added as data_files to the package. A function will copy them from the package to a provided path.

Do not provide __name__ to dash.Dash fixes the assets loading problem because __name__ is now within the installed package and not anymore linked to current directory

smartie2076

One comment to the installation instructions: Those are user-focused in README.md, aren`t they? Is there a link somewhere that points to the dev installation procedure, or is there no difference?

Wouldnt it make sense to directly read from the README.rst and load the installation procedure into Installation.rst?

src/multi_vector_simulator/utils/__init__.py

src/multi_vector_simulator/F2_autoreport.py

smartie2076 · 2020-10-21T10:24:50Z

src/multi_vector_simulator/F2_autoreport.py

                        className="cell imp_info",
                        children=[
                            html.P(f"MVS Release: {version_num} ({version_date})"),
-                            html.P(f"Branch-id: {branchID}"),


But printing the used branch in the report was important for me, so that we do not mess up comparing two scenarios that are used with different dev versions or something. Is removing it necessary?

If you run the simulation after installing the package with pip, you might run a simulation not from the root of this repository, therefore the code to fetch the branch id of this repository fails because you are not anymore within a repository when you run the code.

Ultimately, the version number is the only thing which matters (it is uniquely mapped to a commit id in the master branch of the repo) as on pip you cannot have two release with same version numbers, you can only bump version number up.

If you want to keep the branch-id feature, we should pack this into a function of utils which detects if the person executing the code does it from the root of the repository or not and provide, or not, the branch id

Mhm... okay. Actually, as the end-user will use the simplest way to install the MVS, we probably do not need to print the branch-id.

For devs: When working on the dev branch, we have the same version number as the master release, dont we? Could we not bump the version number of dev to something like vX.Y.Z.dev? and then we are at least warned when we compare dev to real releases. Additionally, we can then check the simulation date to guess if large changes might already have been applied to dev...

We can reintroduce the ID in a subsequent PR what do you say @smartie2076 ?

You mean as in "introduce in another way"? Sounds fine. Can you create an issue noting down your thoughts?

Otherwise it would be better to just revieve it now.

src/multi_vector_simulator/utils/constants.py

src/multi_vector_simulator/cli.py

README.md

CHANGELOG.md

smartie2076 · 2020-10-21T10:45:07Z

CHANGELOG.md

+- If default folder does not exist when code is executed, example simulation's inputs are used from package_data (#607)

 ### Removed
+- Remove reference to git branch ID in the report (#607)


Because you are within this repository only when you execute the code from somewhere else there is no repository

Ok, I got the idea. I hope there wont be too many mess-ups when we as devs simulate the pilots with the dev version. Maybe we should just prohibit people to do that xD

smartie2076 · 2020-10-21T14:54:07Z

Does this PR address that currently the tables are not displayed in the pdf report? See here: #606 (comment)

Bachibouzouk · 2020-10-21T15:27:45Z

Does this PR address that currently the tables are not displayed in the pdf report? See here: #606 (comment)

I can see the tables with code from this PR

smartie2076 · 2020-10-21T15:29:19Z

Does this PR address that currently the tables are not displayed in the pdf report? See here: #606 (comment)

I can see the tables with code from this PR

"Tables with code" You mean, the tables with values? Nice.

Bachibouzouk · 2020-10-21T15:30:57Z

Does this PR address that currently the tables are not displayed in the pdf report? See here: #606 (comment)

I can see the tables with code from this PR

"Tables with code" You mean, the tables with values? Nice.

--> "I can see tables" + "with the code from this PR"

Bachibouzouk force-pushed the fix/mvs-path branch from 296c9d0 to 672b8d6 Compare October 14, 2020 09:48

Bachibouzouk mentioned this pull request Oct 14, 2020

Finalize packaging mvs #608

Merged

7 tasks

Bachibouzouk marked this pull request as ready for review October 14, 2020 20:46

Bachibouzouk requested a review from smartie2076 October 14, 2020 20:46

Bachibouzouk and others added 18 commits October 19, 2020 12:58

Replace REPORT_PATH by argument path_sim_output

9de75d4

Create a util function to copy the asset folder from repository

e93de08

The eland logo and styles.css files are added as data_files to the package. A function will copy them from the package to a provided path.

Implement create_app changes in mvs_report

3db30a9

Make repo_path relative to current directory

0858f87

Lint with black

9b729d3

Replace forgotten REPO_PATH, "assets" by asset_folder variable

d308f9f

Remove reference to git branch head commit number

70b048e

Change type of dict_values[PATHS_TO_PLOTS][PLOTS_ES] to str

d490581

Fix dash app creation

452f300

Do not provide __name__ to dash.Dash fixes the assets loading problem because __name__ is now within the installed package and not anymore linked to current directory

Add report parsing arguments

c34ad45

Move report arg parser to A0

11902f8

Move mvs_report to cli

d00186e

Add default simulation input in package data

c5efab3

Define entry points

aca3adf

In case the default folder does not exists, use package data

0ba7d20

Fix failing test

7a38d9f

Update documentation

8124eec

Update changelog

1d46514

Bachibouzouk force-pushed the fix/mvs-path branch from 0a6e7c8 to 1d46514 Compare October 19, 2020 10:58

smartie2076 reviewed Oct 21, 2020

View reviewed changes

Bachibouzouk added 5 commits October 21, 2020 16:21

Rename PATH_SIM_OUTPUT to ARG_PATH_SIM_OUTPUT

39def38

Elaborate on docstring parameter

6c492de

Rename create_parser to mvs_arg_parser

6adaef9

Update parsers docstrings

b7df6e5

Correct typo in README

c06d115

Update changelog

1f71fb5

This was referenced Oct 21, 2020

Warn user executing the mvs_tool command without input folder #614

Closed

The id-branch does not make sense for package mvs but is still nice for devs #615

Open

Bachibouzouk requested a review from smartie2076 October 21, 2020 16:05

smartie2076 approved these changes Oct 21, 2020

View reviewed changes

Bachibouzouk merged commit 5ced113 into dev Oct 21, 2020

Bachibouzouk deleted the fix/mvs-path branch October 21, 2020 16:09

smartie2076 mentioned this pull request Nov 10, 2020

Release/v0.5.1 #648

Merged

Conversation

Bachibouzouk commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bachibouzouk commented Oct 14, 2020

Uh oh!

smartie2076 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smartie2076 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Bachibouzouk Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

smartie2076 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Bachibouzouk Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smartie2076 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smartie2076 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Bachibouzouk Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

smartie2076 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

smartie2076 commented Oct 21, 2020

Uh oh!

Bachibouzouk commented Oct 21, 2020

Uh oh!

smartie2076 commented Oct 21, 2020

Uh oh!

Bachibouzouk commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Bachibouzouk commented Oct 14, 2020 •

edited

Loading

Bachibouzouk Oct 21, 2020 •

edited

Loading