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

Update SSB towards a release #31

Merged
merged 18 commits into from
Mar 9, 2021
Merged

Conversation

uellue
Copy link
Member

@uellue uellue commented Feb 24, 2021

  • Simplification of API
  • Always use wavelength, do conversion from electron energy outside of UDF
  • Example notebook
  • Docstrings
  • Adapt test to API changes
  • General docs revision and update
  • Moved some code to prototypes, possibly to be worked on later
  • Removed authorship policy since other partners didn't see a need.

Closes #30

Contributor Checklist:

  • I have added myself to the creators.json file
  • [N/A] I have added a changelog entry for my contribution
  • I have added/updated documentation for all user-facing changes
  • I have added/updated test cases

* Light refactoring to simplify
* Keep accelerating voltage out of SSB implementation, only work with wavelength
* Docstrings
@uellue uellue added the WIP label Feb 24, 2021
Interestingly, working with more depth and relatively small tiles
was a bit faster thann GB-sized tiles. Smaller tiles help to avoid
running out of GPU RAM.
This brings a 2x speed-up for the CPU version when I tested it.
This would require a bit more documentation and unit tests
to make it ready for releasing.

Originally uploaded by @heidemeissner in commit b059ac4
Also remove a document with questions. One can open an Issue
if this is still relevant.
@sk1p
Copy link
Contributor

sk1p commented Feb 25, 2021

This looks already quite good. The ssb module is now a bit on the long side, should we maybe extract the trotter/mask generation into its own module? So have ptychography.reconstruction.ssb.udf and ptychography.reconstruction.ssb.masks, and re-export the UDF (and maybe the main mask functions) from the ssb module. Thoughts?

@uellue
Copy link
Member Author

uellue commented Feb 25, 2021

This looks already quite good. The ssb module is now a bit on the long side, should we maybe extract the trotter/mask generation into its own module? So have ptychography.reconstruction.ssb.udf and ptychography.reconstruction.ssb.masks, and re-export the UDF (and maybe the main mask functions) from the ssb module. Thoughts?

Sounds sensible to me! I'll do that. :-)

Thx @sk1p for the suggestion!
Removed a lot of "lorem ipsum", tried to find a structure that makes
sense for the release of SSB.
@uellue uellue removed the WIP label Feb 25, 2021
@uellue
Copy link
Member Author

uellue commented Feb 25, 2021

@sk1p The docs build clean on my system now. This PR should now be in a state that establishing a CI pipeline and homepage makes sense. How should we go on about that? :-)

@sk1p
Copy link
Contributor

sk1p commented Feb 25, 2021

The docs build clean on my system now. This PR should now be in a state that establishing a CI pipeline and homepage makes sense. How should we go on about that? :-)

  • we should merge over the CI configuration of the LiberTEM repo
  • including the release scripts etc.
  • the azure pipeline definition needs to be cleaned up a bit:
    • distributed docker tests are not needed (yet?)
    • no nodejs tests either
  • then we can configure a Azure project for Ptychography 4.0 on dev.azure.com
  • we also need to enable the codecov.io thingy for this repo
  • Edit by @uellue: Create Zenodo sandbox initial deposition
  • Edit by @uellue: Create Zenodo production initial deposition
  • Edit by @uellue: Create PyPI sandbox and production initial project?
  • We then need to make some secrets available to the pipeline:
    • Note @uellue: I have changed the variable names from LT_* to PTY_* in azure-pipelines.yml
    • for example, the deploy_docs job needs to have access to a github deploy key
    • and we need to generate access tokens for test.pypi.org (libertem_bot user?)
    • and for pypi.org
    • and for sandbox.zenodo.org
    • and for zenodo.org
  • we can then also add links on https://github.com/liberTEM/libertem.github.io (it's very simple, just change the readme.md)
  • for tests w/ data access, need to add a second azure pipeline, possibly with its own runner (not sure the other runner can/should be made available to different projects)

@uellue
Copy link
Member Author

uellue commented Feb 26, 2021

I'm wondering if we should run the CI pipeline on all Python versions and all OSes. So far, we only depend on LiberTEM, NumPy, Numba, CuPy, SciPy, and sparse directly. CuPy and LiberTEM would be the most likely source of system-dependent trouble in my experience. We can't test CuPy in CI anyway, and LiberTEM is already tested against different systems in its own CI pipeline. Since we do nothing out of the ordinary with LiberTEM for now, I'm not sure what the probability would be to hit any system-dependent issues here.

The only argument would be that we run the latest release of LiberTEM here, which means we'd catch regressions from upstream changes which may not affect the current LiberTEM master, like the one that prompted us to release version 0.5.1.

Since the CI pipeline here is still quite short, perhaps it makes sense to run it against all versions and OSes just to catch regressions affecting the latest release of LiberTEM, until we have a better solution to cover that?

@uellue uellue added this to the 0.1 milestone Feb 26, 2021
@sk1p
Copy link
Contributor

sk1p commented Feb 26, 2021

test_requirements.txt may need an update, too.

Since the CI pipeline here is still quite short, perhaps it makes sense to run it against all versions and OSes just to catch regressions affecting the latest release of LiberTEM, until we have a better solution to cover that?

Sounds good to me.

We can't test CuPy in CI anyway, [...]

In the "data tests", we should be able to use CUDA/cupy, actually, if we take care that the tests don't take up too many resources (GPU memory). We would need to run the Azure agent with CUDA support. It would be slightly harder to support multiple CUDA versions - for the beginning, I think it's better to test on only one "standard" cupy version than not testing at all. Maybe HZDR/@SimeonEhrig could also run an Azure agent for this repo?

* Purge unneeded items from test setup
* Align with current LiberTEM
* Add an example so that `--doctest-modules` is happy
@uellue
Copy link
Member Author

uellue commented Feb 26, 2021

test_requirements.txt may need an update, too.

Good point! I've purged a lot of LiberTEM-specific items from the test setup.

@uellue
Copy link
Member Author

uellue commented Feb 26, 2021

if we take care that the tests don't take up too many resources (GPU memory)

Good point! The UDF was updated in this PR to use little GPU RAM and not more than is available. 👍

@SimeonEhrig
Copy link
Contributor

test_requirements.txt may need an update, too.

Since the CI pipeline here is still quite short, perhaps it makes sense to run it against all versions and OSes just to catch regressions affecting the latest release of LiberTEM, until we have a better solution to cover that?

Sounds good to me.

We can't test CuPy in CI anyway, [...]

In the "data tests", we should be able to use CUDA/cupy, actually, if we take care that the tests don't take up too many resources (GPU memory). We would need to run the Azure agent with CUDA support. It would be slightly harder to support multiple CUDA versions - for the beginning, I think it's better to test on only one "standard" cupy version than not testing at all. Maybe HZDR/@SimeonEhrig could also run an Azure agent for this repo?

I'm not sure, if I understand it correctly. Do mean that we need a private hosted GPU runner for the Azure CI, like we do it on GitLab? And do you really mean Azure or GitHub actions?

@sk1p
Copy link
Contributor

sk1p commented Mar 1, 2021

I'm not sure, if I understand it correctly. Do mean that we need a private hosted GPU runner for the Azure CI, like we do it on GitLab?

That was the idea. We can run one of those with just a single turing card, but maybe in the long run it would be possible to test on more hardware.

And do you really mean Azure or GitHub actions?

Right now we are using Azure Pipelines and have a working configuration for that, if only for the reason that GitHub Actions weren't publicly available when we started the switch on our main LiberTEM repository. If you prefer GitHub Actions, we could do a quick hacking session and port our CI over to Actions, we just didn't have a reason for that yet.

@SimeonEhrig
Copy link
Contributor

I'm not sure, if I understand it correctly. Do mean that we need a private hosted GPU runner for the Azure CI, like we do it on GitLab?

That was the idea. We can run one of those with just a single turing card, but maybe in the long run it would be possible to test on more hardware.

At the moment, we have also a single Turing card. If I'm not wrong, we should get some A100 for the CI this year.

And do you really mean Azure or GitHub actions?

Right now we are using Azure Pipelines and have a working configuration for that, if only for the reason that GitHub Actions weren't publicly available when we started the switch on our main LiberTEM repository. If you prefer GitHub Actions, we could do a quick hacking session and port our CI over to Actions, we just didn't have a reason for that yet.

I need to talk with our CI admin what is possible. But I see two problems at the moment besides the lack of resources, but the A100 should come ;-)

  1. We have no experience with Azure Cloud. We have full experience with GitLab CI and partially with GitHub Actions.
  2. I am not sure if we can run an Azure runner and a GitLab CI runner in parallel on the same HW. If that is not possible, we will stay on GitLab-CI.

I think a realistic solution will be to put as much of the CI as possible into generic (bash-) scripts and have only a few CI specific lines of code. This would allow us to use different CI systems in parallel. This is what we have planned and partially implemented for the alpaka project. There we want to combine the free Github action runners with our GPU runners.

@sk1p sk1p merged commit 9648f27 into Ptychography-4-0:master Mar 9, 2021
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.

Missing requriement documentation for the SSB prototype
3 participants