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

Fix setup to respect other plugins #44

Closed
wants to merge 1 commit into from
Closed

Conversation

ndaelman-hu
Copy link
Collaborator

Clarify use of include and exclude in the setup instructions.
Should we maybe just refer to the general docs for the setup?

@ndaelman-hu ndaelman-hu added documentation Improvements or additions to documentation improvement/fix Improvement or fix of a previous feature labels Apr 9, 2024
@ndaelman-hu ndaelman-hu self-assigned this Apr 9, 2024
Copy link
Collaborator

@JosePizarro3 JosePizarro3 left a comment

Choose a reason for hiding this comment

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

I can work on improving this README part on the local installation setup. It is just not the moment now.

Please, leave this as a PR for now, we can work it out next week or in a couple of weeks.


```yaml
plugins:
include:
Copy link
Collaborator

@JosePizarro3 JosePizarro3 Apr 10, 2024

Choose a reason for hiding this comment

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

Please, we can leave this here. It is just about probably adding the comment that you can find in the nomad.yaml if you feel so.

Furthermore, it seems that manually installing nomad-lab[parsing] fixes the built-in plugins.

Copy link
Collaborator Author

@ndaelman-hu ndaelman-hu Apr 10, 2024

Choose a reason for hiding this comment

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

Well, this is the only correction, so no need to further work on it: I haven't seen any other errors, and have verified that this here works...
You can ask Lauri to take a look.

Copy link
Collaborator Author

@ndaelman-hu ndaelman-hu Apr 10, 2024

Choose a reason for hiding this comment

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

Just noticed this comment now:

Please, we can leave this here. It is just about probably adding the comment that you can find in the nomad.yaml if you feel so

No, this setup disables ALL other plugins, which should never be the default! There are only a few scenarios where this is wishful. You only need options to add plugins to the list, then you can run all of them.
I do think that this has not been properly explained in the general docs. I'll see to amend that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to double-check, @JFRudzinski or @Bernadette-Mohr, following the old setup, can you still run the regular parsers?
Not the tests, but the actual parsers.

@JosePizarro3 JosePizarro3 marked this pull request as draft April 10, 2024 07:44
@ndaelman-hu ndaelman-hu marked this pull request as ready for review April 10, 2024 08:19
@JosePizarro3
Copy link
Collaborator

@ndaelman-hu

I am curious about which exact problems you are having now. We should simply change the text pointing to the diffs between having a local installation and a NOMAD oasis, and I agree this is in the general docu.

Here we just fix the text.

Did you also check the general documentation page? More specifically, this one: https://nomad-lab.eu/prod/v1/staging/docs/howto/oasis/plugins_install.html#add-a-plugin-to-nomad

@JosePizarro3 JosePizarro3 marked this pull request as draft April 10, 2024 09:01
@ndaelman-hu
Copy link
Collaborator Author

@ndaelman-hu

I am curious about which exact problems you are having now. We should simply change the text pointing to the diffs between having a local installation and a NOMAD oasis, and I agree this is in the general docu.

Here we just fix the text.

Did you also check the general documentation page? More specifically, this one: https://nomad-lab.eu/prod/v1/staging/docs/howto/oasis/plugins_install.html#add-a-plugin-to-nomad

The issue is that the README instructions give undesired results, i.e. I can no longer utilize the other plugins, such as parsers, after applying these steps.
The use of include is superfluous here, period. All the rest is fine. Rather than fully removing include, I now added an explanation on its proper use, as is given in the link that you posted. Still, an explanation should not be included as a comment in a code snippet.

@JosePizarro3
Copy link
Collaborator

Given that the setup has changed, and now it works without conflict, I am closing this PR.

@JosePizarro3 JosePizarro3 deleted the fix_setup branch October 15, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation improvement/fix Improvement or fix of a previous feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants