Added and integrated a configuration model #164
Conversation
Reduced CodeCov spam feat: rollback sanitization for split PR
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
==========================================
- Coverage 87.39% 86.71% -0.69%
==========================================
Files 27 32 +5
Lines 960 1001 +41
==========================================
+ Hits 839 868 +29
- Misses 121 133 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Removed release notes for version 0.6.0 and added version 0.5.1 section.
|
@candleindark Some slight effects from a strange merge but cleaned up now, wouldn't worry about the remaining artifacts |
… annotations Redefine `_parent_run_directory` and `_run_directory` of `RunConfig` as private properties since they are computed based on the value of a field of the model and are never modified. Additionally, `None` type annotations are removed from these properties for they are really never `None`
… annotation Set the default of `run_id` using the default factory mechanism. This allows a more declarative definition of the model and `model_post_init` should be reserved for setting something that requires multi-field access. Additionally, the `None` type annotation is removed. `run_id` was never `None` once it was set. The `None` value was only used as an indication of absence of user provided value and to signal the setting of a default. Setting the default of `run_id` using the default factory mechanism obviates the `None` type annotation.
…one` type annotation Set the default of `cache_directory` using the default factory mechanism. This allows a more declarative definition of the model and `model_post_init` should be reserved for setting something that requires multi-field access. Additionally, the `None` type annotation is removed. `cache_directory` was never `None` once it was set. The `None` value was only used as an indication of absence of user provided value and to signal the setting of a default. Setting the default of `cache_directory` using the default factory mechanism obviates the `None` type annotation.
…ne` type annotation Set the default of `bids_directory` using the default factory mechanism. This allows a more declarative definition of the model and `model_post_init` should be reserved for setting something that requires multi-field access. Additionally, the `None` type annotation is removed. `bids_directory` was never `None` once it was set. The `None` value was only used as an indication of absence of user provided value and to signal the setting of a default. Setting the default of `bids_directory` using the default factory mechanism obviates the `None` type annotation.
…to"` value option Set the default of `file_mode` using the default factory mechanism. This allows a more declarative definition of the model and `model_post_init` should be reserved for setting something that requires multi-field access. Additionally, the `"auto"` value option is removed. `file_mode` was never `"auto"` once it was set. The `"auto"` value was only used as an indication of absence of user provided value and to signal the setting of a default. Setting the default of `file_mode` using the default factory mechanism obviates the `"auto"` value option and allows the type annotation of the `file_mode` field to be consistent with the possible values that it holds in operation.
…dator Doing it this way makes the definition of the model more declarative and also avoids using `model_post_init` which should be preserved for validations that requires access to multiple fields of the model
Doing this allows CLI arguments to remain the same to users while making the `RunConfig` more succinct. For example, `RunConfig.run_id` can now be of type `str` instead of `str | None` even though it doesn't hold a value of `None` in operation.
When validating a directory as a BIDS directory, ensure it is a file instead of just ensuring its mere existence. This rules out the situation where `dataset_description.json` is a directory
…json file when validating a directory as a BIDS directory
Co-authored-by: Copilot <[email protected]>
Revise config model
…out to core; add test for immutability; clean CLI code
candleindark
left a comment
candleindark
left a comment
There was a problem hiding this comment.
Left some comments. Possible more to come.
candleindark
left a comment
candleindark
left a comment
There was a problem hiding this comment.
Submitting some small changes/comments.
It should be ready once all the requests are addressed.
| Only light-weight configuration files are kept here. | ||
| Heavier contents (such as text files, logs, etc.) are stored in the cache directory. |
There was a problem hiding this comment.
Possible adjustment to this docs string is needed because the only use of the return directory of this function is as the cache directory
.There was a problem hiding this comment.
Sorry, can you clarify what you mean here? This is a simple util function to reduce duplicated calls to the home path (which is used in more than one place across various incoming PRs)
There was a problem hiding this comment.
The doc string seems to suggest that the return of this function, the home directory, is different than the cache directory. Specifically, it states that the home directory is for light-weight files, and the cache directory is for heavier contents. However, this home directory is being used as the default of the cache directory, so the home directory is the cache directory of the user doesn't specify a cache directory, contradicting what is suggested by the doc string.
There was a problem hiding this comment.
The doc string seems to suggest that the return of this function, the home directory, is different than the cache directory.
It is 😊
Specifically, it states that the home directory is for light-weight files, and the cache directory is for heavier contents.
Glad the idea got across!
However, this home directory is being used as the default of the cache directory, so the home directory is the cache directory of the user doesn't specify a cache directory
Correct, there is currently there is no persistent cache directory (only what is specified at RunConfig instantiation). Was going to add that in follow-ups and make the distinction more formal. Didn't want to overwhelm an already very busy PR
If you really care we can remove this line, but it will simply be added back in very soon after
Co-authored-by: Isaac To <[email protected]>
Co-authored-by: Isaac To <[email protected]>
|
@candleindark Ready for another round The docstring suggestion can also be a follow-up PR if you have clear idea what you want |
I follow up about the doc string in #164 (comment). I actually don't know the exact intention. I just see a contradiction. I think it would be best to resolve it in this PR since the Anyway, this PR is good to go. Including a doc string adjustment would be a plus. |
| file_mode : one of "move", "copy", or "symlink" | ||
| Specifies how to handle the NWB files when converting to BIDS format. | ||
| - "move": Move the files to the BIDS directory. | ||
| - "copy": Copy the files to the BIDS directory. |
There was a problem hiding this comment.
note mentioning somewhere that it would be CoW if filesystem supports and Python 3.13+.
| with preference for linking when possible. | ||
| cache_directory : directory path | ||
| The directory where run specific files (e.g., notifications, sanitization reports) will be stored. | ||
| Defaults to `~/.nwb2bids`. |
There was a problem hiding this comment.
worth making it relative and make that relative to the top of the bids directory. BIDS (validator) ignores all .dotdirs so there should be no side effect.
This would be inline with e.g. heudiconv functioning where we store metadata extracts etc, see e.g https://datasets.datalad.org/?dir=/dbic/QA (but need explicit path) -- frontend ignores .dotdirs so need to go directly into apache's index: https://datasets.datalad.org/dbic/QA/.heudiconv/ but it will be there if cloned locally.
What changed
Release Notes
This release incudes big changes to how arguments are passed in the
nwb2bidsAPI: theRunConfigobject.This class is a Pydantic model which encapsulates all previous configuration settings, such as the output BIDS directory and the additional metadata file path. This class is also now passed at time of initialization for all
Converterclasses and prior to calling theconvert_nwb_datasethelper function. This reduces any confusion about which steps of the workflow take which arguments, and allows all internal actions to refer to the common location instead of having to manage passing values back-and-forth. It also has the added benefit of simplifying any future additions to configuration options, such as sanitization parameters.CLI users are unaffected by these changes, aside from gaining a few new arguments - check them out with