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.

Sign up for GitHub

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

Add explanatory text on tests #124

Merged
merged 3 commits into from
Jan 10, 2025
Merged

Add explanatory text on tests #124

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
daebebb
Add some text on tests
ferrigno Jan 8, 2025
82972a8
explain better the tests
ferrigno Jan 8, 2025
26ea8a4
MMODA dispatcher not nb2workflow
ferrigno Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions content/docs/guide-development.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,13 @@ os.environ['ODA_SECRET_STORAGE'] = '/secrets_custom'

It is a good practice to test the developed notebook. This allows to make sure that the code remains valid in the future.
A test is implemented as another notebook, except that name of the notebook starts with "test_". The notebook should call other notebooks and check that the output matches expectations. See an example of such a test [here](https://gitlab.renkulab.io/astronomy/mmoda/mmoda-nb2workflow-example/-/blob/master/notebooks/test_lightcurve.ipynb).
To run tests, it is necessary to add some dependencies to the basic environment: in `environment.yml`, add libmagic see [this example](https://gitlab.renkulab.io/astronomy/mmoda/jemx-expert/-/blob/main/environment.yml?ref_type=heads); in `requirements.txt` add the `dispatcher-plugin-nb2workflow` from the git repository, see the last line of [this example](https://gitlab.renkulab.io/astronomy/mmoda/jemx-expert/-/blob/main/requirements.txt?ref_type=heads).
Copy link
Member

Choose a reason for hiding this comment

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

Why libmagic here? Isn't it specific to jemx-expert?

Concerning dispatcher-plugin-nb2workflow, it helps to look on how dispatcher "understands" the workflow and possible "catch" improper annotations. But it's not needed to run a test notebook.
The downside of having it in the requirements is that it will be installed in the container, blowing its size and complicating dependencies.

Copy link
Contributor Author

@ferrigno ferrigno Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi Denys, thanks for this comment. We had already part of this discussion, but it is important to review our understanding.

  1. libmagic is required by nb2workflow to run the notebook possibly through the plugin (see missing compulsory argument in nbadapter call to NB2WProductQuery from dispatcher-plugin-nb2workflow nb2workflow#199 (comment) )
  2. without dispatcher-plugin-nb2workflow nb2workflow run did not complete (see missing compulsory argument in nbadapter call to NB2WProductQuery from dispatcher-plugin-nb2workflow nb2workflow#199 (comment) )

I would be happy if this could simplify, but this is what we concluded trying to run a test on my notebooks.

Copy link
Member

Choose a reason for hiding this comment

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

  1. OK, this dependency problems need to be resolved in the package Possibly dependencies problem with libmagic dispatcher-plugin-nb2workflow#124
    Having some dependencies pinned in the repo is sometimes necessary, but I wouldn't recommend putting this into docs. In this case, it's a kind of hotfix.

Copy link
Member

@dsavchenko dsavchenko Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. This is just a warning,
    then all is skipped, unless optional_dispather=False in nbrun
    (controlled with --mmoda_validation flag if run from command line)

Copy link
Member

Choose a reason for hiding this comment

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

I now better understand this libmagic issue. libmagic itself is not a python package. Therefore, it's not possible to ensure its presence in the container via python package dependencies, only with conda (or modifying Dockerfile and installing with system package manager). Then the advise to install it makes sense. But it must be formulated the way it's obvious that it's only needed if there is a specific problem with dispatcher plugin.

And I still think dispatcher plugin is not strictly required there

The test consists of:
1. load the nb2workflow runner (`from nb2workflow.nbadapter import run`)
2. prepare a dictionary of the input parameters (`par_dict={...}`)
3. running a notebook (`test_result = run('my_notebook.ipynb', par_dict)`)
4. accessing results from `test_result`, which is a dictiory whose keys are the names of the output variables (e.g. `result = test_result['result'])
5. Make possible assessments on results

### Reporting progress for long running tasks

Expand Down
Loading