[ENH]: Adopt Pydantic for schema validation #364

Same as below.

With this change, now this line looks a bit lost in the docs. Maybe we should explicitly show that this is now a class attribute.

docs/using/codeless.rst

I will stop commenting on this

examples/run_datagrabber_bids_datalad.py

The example should work without this "complicated" things from Pydantic.

junifer/api/functions.py

Why here and not in line 186? Why is not done in the __init__ method of the WorkDirManager?

Why here and not in line 186? Why is not done in the `__init__` method of the `WorkDirManager`?

					
				@ -19,0 +25,4 @@

				_types = Literal[

				    DataType.BOLD,

				    DataType.T1w,

				    DataType.VBM_CSF,

should be a DataType of a list of them

Same for all the datagrabbers, markers, steps, etc where it can be one or many.

					
				@ -21,2 +25,3 @@

				    """Abstract base class for DataGrabber.

				class DataType(str, AEnum):

				    """Accepted data type."""

Now we hit an important issue here.

How can I create a junifer extension that allows to process a new DataType (e.g. EEG)?

Now we hit an important issue here. How can I create a junifer extension that allows to process a new DataType (e.g. EEG)?

without an _ it will be considered part of the metadata for the computation of the hash. For Datalad-based datagrabbers, this should not count.

without an `_` it will be considered part of the metadata for the computation of the hash. For Datalad-based datagrabbers, this should not count.

I think that is why it's a property.

					
				@ -87,0 +140,4 @@

				            self._repodir.mkdir(parents=True, exist_ok=False)

				            logger.info(

				                "Datalad dataset installation path set to: "

				                f"{self._repodir.resolve()!s}"

this is horrible, what if the datadir was set by the user to datalad_dataset_aomic?

this is horrible, what if the datadir was set by the user to `datalad_dataset_aomic`?

It's missing the space in case it's dirty.

					
				@ -18,1 +22,4 @@

				_types = Literal[DataType.BOLD, DataType.T1w, DataType.Warp]

will this expand? Otherwise I prefer the previous docstring as it tells you exactly what your options are.

2025-11-27 07:16:28 +00:00

Having an enum for the confounds format will also restrict someone defining their own format and using their own preprocessor for that format.

synchon (Migrated from github.com) reviewed

2025-11-27 07:22:30 +00:00

Here's the present situation where one can't extend it: github.com/juaml/junifer@65be1297f8/junifer/datagrabber/pattern.py (L195-L203) . With this change they wouldn't be able to extend it either but can be made so with a couple of lines (see DataType).

Here's the present situation where one can't extend it: https://github.com/juaml/junifer/blob/65be1297f800bfe922a4d41848d66ab00cf2b195/junifer/datagrabber/pattern.py#L195-L203 . With this change they wouldn't be able to extend it either but can be made so with a couple of lines (see `DataType`).

synchon (Migrated from github.com) reviewed

2025-11-27 07:24:41 +00:00

					
				@ -18,1 +22,4 @@

				_types = Literal[DataType.BOLD, DataType.T1w, DataType.Warp]

Here's the doc link: https://juaml.github.io/junifer/pr-preview/pr-364/api/datagrabbers.html#junifer.datagrabber.DataladHCP1200 and here's the doc link for the enum: https://juaml.github.io/junifer/pr-preview/pr-364/api/datagrabbers.html#junifer.datagrabber.HCP1200Task

synchon (Migrated from github.com) reviewed

2025-11-27 07:28:09 +00:00

Without the change it prints double space in case it's dirty and with the change it does single space.

synchon (Migrated from github.com) reviewed

2025-11-27 07:42:43 +00:00

Here's where it gets removed and does not become part of hash computation: github.com/juaml/junifer@0f2ecf0ce7/junifer/storage/utils.py (L81-L86)

Here's where it gets removed and does not become part of hash computation: https://github.com/juaml/junifer/blob/0f2ecf0ce7082248a3c8ba0c9136bf62860620b2/junifer/storage/utils.py#L81-L86

synchon (Migrated from github.com) reviewed

2025-11-27 07:56:12 +00:00

					
				@ -21,2 +25,3 @@

				    """Abstract base class for DataGrabber.

				class DataType(str, AEnum):

				    """Accepted data type."""

Same way as one does now.

synchon (Migrated from github.com) reviewed

junifer/api/functions.py

Why here and not in line 186?

Nothing gets created until the datagrabber object is accessed, hence kept here. I don't have a problem moving it to L186.

Why is not done in the init method of the WorkDirManager?

It gives RecursionError if kept there.

> Why here and not in line 186? Nothing gets created until the datagrabber object is accessed, hence kept here. I don't have a problem moving it to L186. > Why is not done in the __init__ method of the WorkDirManager? It gives RecursionError if kept there.

synchon (Migrated from github.com) reviewed

2025-11-27 08:15:17 +00:00

2025-11-28 09:38:49 +00:00

					
				@ -87,0 +140,4 @@

				            self._repodir.mkdir(parents=True, exist_ok=False)

				            logger.info(

				                "Datalad dataset installation path set to: "

				                f"{self._repodir.resolve()!s}"

Hmm that's a fair argument, I can do something like:

datadir = WorkDirManager().get_tempdir(
        prefix="datalad", suffix="juniferauto"
)

and then check with:

if self.datadir.stem.startswith(
    "datalad"
) and self.datadir.stem.endswith("juniferauto"):

Hmm that's a fair argument, I can do something like: ```python datadir = WorkDirManager().get_tempdir( prefix="datalad", suffix="juniferauto" ) ``` and then check with: ```python if self.datadir.stem.startswith( "datalad" ) and self.datadir.stem.endswith("juniferauto"): ```

synchon commented

(Migrated from github.com)

Many parameters were str or list of str. Now they are lists. This adds the unnecessary burden to add a list in the YAMLs where the most common use case is to use a single element.

All of them should be reverted now, let me know if you find any stray ones.

some parameters were str or Path and now they have to be Paths. This also makes it more complicated for users to define code.

Not at all, users can keep passing str and they'll be converted to Path by pydantic.

This can easily be validated later on and not with pydantic. Same for HttpUrl

The introduction of pydantic is primarily to not hand-roll these validations anymore and users should only need to add logical validations, but anyway these can be easily added via pydantic. HttpUrl is now AnyUrl and is same as the Path argument above.

I think there's an issue with the datadir which now will be part of the meta due to missing it's _.

Has been addressed in the specific comment.

Also, there was a reason why it was not called fulldir in the datababber, but datadir, on purpose.

The reason was to keep it in sync with non-datalad based datagrabbers and is already taken care of internally via introduction of _repodir. The UX should be exactly as it is now, let me know if you find any difference while using.

Some enums might add validation to pydantic, but will restraint the extensibility of junifer. E.g. DataType, ConfoundFormat, etc... These should be dynamic, as I might want to add some of them in an extension, without modifying junifer code.

Has been addressed in specific comments.

> * Many parameters were `str or list of str`. Now they are lists. This adds the unnecessary burden to add a list in the YAMLs where the most common use case is to use a single element. All of them should be reverted now, let me know if you find any stray ones. > * some parameters were `str or Path` and now they have to be Paths. This also makes it more complicated for users to define code. Not at all, users can keep passing `str` and they'll be converted to `Path` by pydantic. > This can easily be validated later on and not with pydantic. Same for `HttpUrl` The introduction of `pydantic` is primarily to not hand-roll these validations anymore and users should only need to add logical validations, but anyway these can be easily added via pydantic. `HttpUrl` is now `AnyUrl` and is same as the `Path` argument above. > * I think there's an issue with the `datadir` which now will be part of the meta due to missing it's `_`. Has been addressed in the specific comment. > Also, there was a reason why it was not called `fulldir` in the datababber, but `datadir`, on purpose. The reason was to keep it in sync with non-datalad based datagrabbers and is already taken care of internally via introduction of `_repodir`. The UX should be exactly as it is now, let me know if you find any difference while using. > * Some enums might add validation to pydantic, but will restraint the extensibility of junifer. E.g. DataType, ConfoundFormat, etc... These should be dynamic, as I might want to add some of them in an extension, without modifying junifer code. Has been addressed in specific comments.

fraimondo (Migrated from github.com) reviewed

2025-12-01 08:05:29 +00:00

2025-12-01 08:06:30 +00:00

but for non-datalad datagrabbers this should be counted.

fraimondo (Migrated from github.com) reviewed

2025-12-01 08:08:09 +00:00

					
				@ -87,0 +140,4 @@

				            self._repodir.mkdir(parents=True, exist_ok=False)

				            logger.info(

				                "Datalad dataset installation path set to: "

				                f"{self._repodir.resolve()!s}"

why can't we keep a private var _was_cloned and check that? What if we "run a test without cleaning the workdir" and then we use it?

The behaviour should be "clean whatever mess you made".

why can't we keep a private var `_was_cloned` and check that? What if we "run a test without cleaning the workdir" and then we use it? The behaviour should be "clean whatever mess you made".

fraimondo (Migrated from github.com) reviewed

2025-12-01 08:11:30 +00:00

[junifer] pool-49-54 ➜ ~  ipython
Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:33:12) [Clang 15.0.7 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.16.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: is_dirty = True

In [2]: print(f"Remote dataset is{'' if is_dirty else ' not '}dirty")
Remote dataset isdirty

In [3]: is_dirty = False

In [4]: print(f"Remote dataset is{'' if is_dirty else ' not '}dirty")
Remote dataset is not dirty

``` [junifer] pool-49-54 ➜ ~ ipython Python 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:33:12) [Clang 15.0.7 ] Type 'copyright', 'credits' or 'license' for more information IPython 8.16.0 -- An enhanced Interactive Python. Type '?' for help. In [1]: is_dirty = True In [2]: print(f"Remote dataset is{'' if is_dirty else ' not '}dirty") Remote dataset isdirty In [3]: is_dirty = False In [4]: print(f"Remote dataset is{'' if is_dirty else ' not '}dirty") Remote dataset is not dirty ```

fraimondo (Migrated from github.com) reviewed

2025-12-01 08:14:01 +00:00

					
				@ -18,1 +22,4 @@

				_types = Literal[DataType.BOLD, DataType.T1w, DataType.Warp]

Ok, so now I found that we have an even bigger problem with the DOC.

This is the "user documentation" where we show what we have available: https://juaml.github.io/junifer/pr-preview/pr-364/builtin.html

When you click on DataladHCP1200, it goes to the API doc, which is then this: https://juaml.github.io/junifer/pr-preview/pr-364/api/datagrabbers.html#junifer.datagrabber.DataladHCP1200

This is terrible for users. types is not a string anymore, neither tasks nor phase_encodings which makes it cryptic. "What shall I put in the field?" The previous docstring was easier to understand E.g. "use the string "REST1" and it will work!"

Ok, so now I found that we have an even bigger problem with the DOC. This is the "user documentation" where we show what we have available: https://juaml.github.io/junifer/pr-preview/pr-364/builtin.html When you click on DataladHCP1200, it goes to the API doc, which is then this: https://juaml.github.io/junifer/pr-preview/pr-364/api/datagrabbers.html#junifer.datagrabber.DataladHCP1200 This is terrible for users. `types` is not a string anymore, neither `tasks` nor `phase_encodings` which makes it cryptic. "What shall I put in the field?" The previous docstring was easier to understand E.g. "use the string "REST1" and it will work!"

fraimondo (Migrated from github.com) reviewed

2025-12-02 05:32:04 +00:00

I still don't understand how can I add a datatype in an extension.

synchon (Migrated from github.com) reviewed

2025-12-02 06:37:47 +00:00

You'd do as shown here: https://juaml.github.io/junifer/pr-preview/pr-364/extending/data_types.html

synchon (Migrated from github.com) reviewed

2025-12-02 08:47:12 +00:00

Should be fixed with the latest commit.

synchon (Migrated from github.com) reviewed

2025-12-02 09:15:23 +00:00

					
				@ -87,0 +140,4 @@

				            self._repodir.mkdir(parents=True, exist_ok=False)

				            logger.info(

				                "Datalad dataset installation path set to: "

				                f"{self._repodir.resolve()!s}"

why can't we keep a private var _was_cloned and check that?

We already have _was_cloned for a similar purpose. I don't think I understand how you want it to be implemented.

> why can't we keep a private var `_was_cloned` and check that? We already have `_was_cloned` for a similar purpose. I don't think I understand how you want it to be implemented.

synchon (Migrated from github.com) reviewed

2026-05-19 09:08:32 +00:00

					
				@ -18,1 +22,4 @@

				_types = Literal[DataType.BOLD, DataType.T1w, DataType.Warp]

Should be addressed with the latest commit.

fraimondo (Migrated from github.com) reviewed

2026-05-19 11:20:26 +00:00

Can you implement the possibility to extend confound formats?

synchon commented

(Migrated from github.com)

@fraimondo I've pushed the further changes, feel free to review.

fraimondo (Migrated from github.com) reviewed

2026-05-26 08:31:31 +00:00

fraimondo (Migrated from github.com) left a comment

Last questions

We can already leave the typing in the template here

Since they are datatypes, shouldn't they be DataType.T1w, etc? Or it is ok to keep them as strings?

junifer/cli/tests/data/gmd_mean_htcondor.yaml

					
				@ -17,4 +17,5 @@ queue:

				  env:

				    kind: conda

				    name: junifer

				    shell: bash

We don't have bash as default here?

2026-05-26 08:44:17 +00:00

This will happen a alot, might be worth having a "util" function to "update patterns".

junifer/markers/parcel_aggregation.py

Do we still need the on here? It's inherited from BaseMarker

Do we still need the `on` here? It's inherited from `BaseMarker`

There are many ocasions in which the on is repeated on the inherited marker.

There are many ocasions in which the `on` is repeated on the inherited marker.

synchon (Migrated from github.com) reviewed

2026-05-26 08:44:51 +00:00

They get automatically converted.

synchon (Migrated from github.com) reviewed

2026-05-26 08:46:14 +00:00

What do you mean?

synchon (Migrated from github.com) reviewed

junifer/cli/tests/data/gmd_mean_htcondor.yaml

					
				@ -17,4 +17,5 @@ queue:

				  env:

				    kind: conda

				    name: junifer

				    shell: bash

Yes the default is bash, this is to be explicit.

synchon (Migrated from github.com) reviewed

2026-05-26 08:47:45 +00:00

junifer/markers/parcel_aggregation.py

Yes we need it to constrain the correct data types else any DataType will be valid.

synchon (Migrated from github.com) reviewed

2026-05-26 08:50:36 +00:00

2026-05-26 08:54:51 +00:00

I agree it has value and it's a dict so it should be straightforward to make one. I wouldn't want to do it in this PR though, might be good to tackle in another one.

fraimondo (Migrated from github.com) reviewed

2026-05-26 08:55:12 +00:00

This is supposed to be a template to create your own preprocessor:

_VALID_DATA_TYPES: ClassVar[Sequence[DataType]] = []

This is supposed to be a template to create your own preprocessor: ``` _VALID_DATA_TYPES: ClassVar[Sequence[DataType]] = [] ```

fraimondo (Migrated from github.com) reviewed

junifer/cli/tests/data/gmd_mean_htcondor.yaml

					
				@ -17,4 +17,5 @@ queue:

				  env:

				    kind: conda

				    name: junifer

				    shell: bash

ok

fraimondo (Migrated from github.com) reviewed

2026-05-26 08:56:27 +00:00