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

Old school version #67

Merged
merged 19 commits into from
Feb 2, 2024
Merged

Old school version #67

merged 19 commits into from
Feb 2, 2024

Conversation

volodymyrss
Copy link
Member

No description provided.

added screenshot of the gitlab ci/cd for MMODA
Added a note on exception handling (Runtime error)
@ferrigno
Copy link
Contributor

ferrigno commented Sep 7, 2023

"We encourage the developers to supply tests that will be automatically executed from time to time during service operations and are supposed to always yield exactly the same results." should contain a reference to a page where it is explained how to make a test. As of now, I would simply skip this point, as I do not not know how to implement a test. @andriineronov do you know how to do it?

@ferrigno
Copy link
Contributor

ferrigno commented Sep 7, 2023

"Note that some of the input parameters in the example of the Fermi/LAT Lightcurve.ipynb notebook appear as multiple choice parameters with pre-defined values, while others are query fields." This is not clear, it needs to be elaborated with a reference or more text, I do not understand why some parameters are multiple choice and what is regulating this. What if I would like to make a multiple choice ?

@volodymyrss
Copy link
Member Author

"Note that some of the input parameters in the example of the Fermi/LAT Lightcurve.ipynb notebook appear as multiple choice parameters with pre-defined values, while others are query fields." This is not clear, it needs to be elaborated with a reference or more text, I do not understand why some parameters are multiple choice and what is regulating this. What if I would like to make a multiple choice ?

it's because of the annotations. It may be good to explain more, look in the annotations to see why it is like so. If unclear, propose some text?

@volodymyrss
Copy link
Member Author

"We encourage the developers to supply tests that will be automatically executed from time to time during service operations and are supposed to always yield exactly the same results." should contain a reference to a page where it is explained how to make a test. As of now, I would simply skip this point, as I do not not know how to implement a test. @andriineronov do you know how to do it?

I added a reference

@dsavchenko
Copy link
Member

I added a reference

But we don't run tests in the current mode of deployment https://github.com/oda-hub/nb2workflow/blob/2d838468464abb22f0770440bd616a54d600a6ce/nb2workflow/deploy.py#L74C26-L74C26

We need to adapt. Run tests as k8s jobs or something?

@ferrigno
Copy link
Contributor

ferrigno commented Sep 7, 2023

"Note that some of the input parameters in the example of the Fermi/LAT Lightcurve.ipynb notebook appear as multiple choice parameters with pre-defined values, while others are query fields." This is not clear, it needs to be elaborated with a reference or more text, I do not understand why some parameters are multiple choice and what is regulating this. What if I would like to make a multiple choice ?

it's because of the annotations. It may be good to explain more, look in the annotations to see why it is like so. If unclear, propose some text?

The multiple choice of Lightcurve, spectrum image is not in the input parameters nor in the "default" parameter block.

What I propose:

Some automatic multiple choice parameters are inserted by default in the product tab: these are .....

If users needs to introduce a multiple choice for string (or any other format), they need to add the annotation ``oda:allowed_value "LIN","LOG"` after the parameter type annotation.

However, these two parts should be added in the respective sections and the point mentioned here. As I do not have all the overview, would you do it, @volodymyrss or should I make a commit and you complete it?

@ferrigno
Copy link
Contributor

ferrigno commented Sep 7, 2023

Overall, the guide is very well written and clear, my comments were just to improve some details. There looks to be still something not clear on tests, though.

@dsavchenko
Copy link
Member

I've added some description for the features not documented yet.
@andriineronov I leave you to add meaningful acknowledgements to Fermi and make the screenshot.

Also, does anyone remember if we have the workflow which actually returns comments via oda:WorkflowResultComment ?

@volodymyrss
Copy link
Member Author

I've added some description for the features not documented yet. @andriineronov I leave you to add meaningful acknowledgements to Fermi and make the screenshot.

Also, does anyone remember if we have the workflow which actually returns comments via oda:WorkflowResultComment ?

I do not remember.
We can add it to the example.

@dsavchenko
Copy link
Member

When do we plan to merge these changes?

@volodymyrss volodymyrss merged commit a27fb60 into master Feb 2, 2024
1 check passed
@volodymyrss
Copy link
Member Author

we waited for remaining reviews but ok.

@volodymyrss
Copy link
Member Author

I merged this then without @ferrigno or @andriineronov re-reading and looks like we added at least one issue doing it, see #66 . A typo it seems (aknowledgements.md), but I do not know which way the typo works here.

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.

4 participants