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

Make SANSND plugin ready for ncrystal v4 #1 #6

Merged
merged 7 commits into from
Jan 29, 2025

Conversation

tkittel
Copy link
Member

@tkittel tkittel commented Jan 29, 2025

Attention in particular @nicriz (and possibly also @XuShuqi77 @marquezj @dddijulio).

(FYI most text here is similar to highness-eu#1 except for the last paragraph)

These changes are needed to make the plugin ready for NCrystal 4, which will be out in a few weeks.

It can already be used in the meantime however, by pip installing the ncrystal-core and ncrystal-python packages which contains release candidates of the new code (but do NOT install the package named just ncrystal until NCrystal4 is out). For refererence, ncrystal-core contains all binaries, headers, cmake, and ncrystal-config -- and ncrystal-python contains the NCrystal python module and all the other command line tools (including nctool).

To install the plugin, you can now simply point pip directly at the repo:

pip install git+https://github.com/highness-eu/ncplugin-SANSND

And then it will work with no further need to tweak environment variables or whatnot.

Of course, until you accept this PR, the above command won't work. But you can try out the code in this PR with:

pip install git+https://github.com/mctools/ncplugin-template.git@ncplugin-fix-SANSND-for-ncrystal4

This PR also adds a unit tests, that ensures a basic functionality test of the plugin on linux/mac/windows.

@nicriz let me know if at some point you need a bit more introduction to all this. For now I migrated the main plugin code, added a little test in NCTestPlugin.cc, and completely left all the mess inside testcode/ alone :-)

@tkittel
Copy link
Member Author

tkittel commented Jan 29, 2025

So the CI is again not running, most likely since there are not yet any workflows in the main branch.

@nicriz
Copy link
Member

nicriz commented Jan 29, 2025

Hi Thomas, thank you for the little intro to the new plugin system. Great news that we can just pip install the plugins, sounds very handy.

I went through the commits and it seems all good. Didn't understand a lot about the infrastructure's updates, but probably it's not something that should concern me.

I tested the plugin ncplugin-fix-SANSND-for-ncrystal4 with the ncrystal 4 release candidates and it looks okay.

It would be definitely interesting to hear more about this new infrastructure. For example, I would be interested in providing access to the 'ncplugin-SANSND_nanodiamond.ncmat' upon installation, without copying the file or ncdatasrc.addCustomSearchDirectory()

P.S. sorry for the garbage in the testcode folder. I promise I will clean it.

@nicriz nicriz merged commit 936b504 into highness-eu:main Jan 29, 2025
@tkittel
Copy link
Member Author

tkittel commented Jan 30, 2025

Hi @nicriz

Thanks for the quick merge and verification that it all works!

Feel free to reach out if you want a video call with a more in-depth intro in a few weeks time. In the mean-time, note that after installing your plugin, you can see with nctool --browse that you indeed have your plugins .ncmat file available (should be something like plugins::SANSND/ncplugin-SANSND_nanodiamond.ncmat). Same goes for all .ncmat files you might dump in your data/ folder. For other kinds of files, you might have to edit the CMakeLists.txt slightly (but get in touch first perhaps since I aim to keep the CMakeLists.txt and pyproject.toml files in the various plugin repos almost pristine, so it will be easier to update all the plugins in the future).

It will be even easier to add a static plugin which only has data files, in that case you can do it without any CMake. I am working on an example here: https://github.com/mctools/ncrystal/tree/tk_reorder_pyproject/examples/plugin_dataonly

One thing to be aware off, is that you now have a file NCTestPlugin.cc, in which you can add some test functions that verifies the plugin works. I will set up a CI workflow in the ncrystal repo which monitors all the "well known" plugins in other repos, to see that they build and load, and also that their test function runs file. That way I can better avoid accidentally breaking a plugin when I create a new ncrystal release.

@nicriz
Copy link
Member

nicriz commented Jan 30, 2025

Hi again,

Yes, you're right, the key was to use the full name "plugins::SANSND/ncplugin-SANSND_nanodiamond.ncmat" in the cfg string. Amazing! I might add one file for each model then.

I saw the new Test functions infrastructure and I will think about a test for the plugin to insert in the CI workflow.

Let's meet in two weeks or so.

@tkittel
Copy link
Member Author

tkittel commented Jan 30, 2025

FYI @nicriz , I just released ncrystal-pypluginmgr 0.0.4, so with that you can simply install your plugin and then afterwards run:

ncrystal-pluginmanager --test SANSND

To verify that the plugin exists, and run it's test function.

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.

2 participants