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

Add validation to TUI. #492

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add validation to TUI. #492

wants to merge 18 commits into from

Conversation

JoeZiminski
Copy link
Member

@JoeZiminski JoeZiminski commented Mar 17, 2025

Description

This PR adds validation functionality to the terminal user interface (TUI). In the TUI there are now two new screens:

  • Validate Project at Path (wraps quick_validate_project) and allows validation of any project at a given filepath.
  • 'Validate' tab at the project manager screen, which performs validation of existing projects (wraps project.validate_project).

Under the hood there is a central ValidateContent container which holds all functionality and is shared between the two screens (with minor changes in display) (similar to configs).

How to test

The quick-validate project documentation page has also been updated (see here for building the documentation) and tests are added.

To test, you can run datashuttle launch in a terminal (see Choosing a Terminal for the best terminals to run in) in a conda environment that contains datashuttle. You can make a fake NeuroBlueprint project and use datashuttle to validate it through the user interface.

What is this PR

  • Bug fix
  • [X ] Addition of a new feature
  • Other

Why is this PR needed?

Python API validate was not available in the TUI

How has this PR been tested?

New tests added that check validation results are displayed on the screen and that all settings are faithfully passed down to the underlying functions.

Is this a breaking change?

No

Does this PR require an update to the documentation?

Yes, Added. Currently the validation on the project manager window is not fully documented, but this will be added in a follow-up PR as it requires some documentation refactoring.

Checklist:

  • [X ] The code has been tested locally
  • [ X] Tests have been added to cover all new functionality
  • [X ] The documentation has been updated to reflect any changes
  • [X ] The code has been formatted with pre-commit

@JoeZiminski JoeZiminski force-pushed the add_validation_to_tui branch from c7a5211 to a4597e6 Compare March 17, 2025 18:06
@JoeZiminski JoeZiminski force-pushed the add_validation_to_tui branch from d5646ee to 3918744 Compare March 17, 2025 21:42
@JoeZiminski JoeZiminski marked this pull request as ready for review March 18, 2025 00:25
@cs7-shrey
Copy link
Contributor

cs7-shrey commented Mar 19, 2025

Hey, @JoeZiminski the code looks and works really nicely. I have added some minor comments.

Just one more thing,
I think we can add some indicator like "Validating..." to show that the validation is in progress as the include_central option takes some time to complete validation and the TUI freezes during that. We can also do something similar to what we did in the "Transferring..." part. At this moment, it would make sense to make some sort of generalization of something like "execute in worker and display loading animation" (apoligies in advance for the vague language). Let me know what you think!

@sumana-2705
Copy link
Contributor

Hello @JoeZiminski,

I have reviewed the changes on my PC, and everything is working perfectly. The updates are clear, well-structured, and easy to understand. I didn't come across any issues in the functionality, everything looks great! 🚀

@@ -26,6 +26,7 @@ def quick_validate_project(
project_path: str | Path,
top_level_folder: Optional[TopLevelFolder] = "rawdata",
display_mode: DisplayMode = "warn",
strict_mode: bool = False,

Choose a reason for hiding this comment

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

To maintain uniformity across all projects, should we consider setting strict_mode=True as the default? This would ensure that only NeuroBlueprint-compliant folders exist, preventing inconsistencies in datasets. Users who need flexibility could still override this by explicitly setting strict_mode=False when calling the function. This is what I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @Transyltooniaa this is a good point. I'm not 100% sure on this but, due to how datashuttle is likely to be used in neuroscience projects, I think it makes sense to assume there will be non-datashuttle folders within the project folder. This is not ideal (it would be nice if all folders / data were in the datatype folders) but unfortunately this is the reality. As such, it would be good to avoid confusing new users with errors related to extra folders they have in their project. Then, they can opt-in to the stricter mode once they understand the details.

@JoeZiminski
Copy link
Member Author

Thanks all for your reviews, they have been super useful!

@JoeZiminski
Copy link
Member Author

@cs7-shrey good point! That is a prime candidate for running in a worker with a TUI updating screen, as validating the central path will be slow!

@JoeZiminski JoeZiminski requested a review from viktorpm March 23, 2025 23:39
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.

5 participants