Skip to content

Conversation

fabiofelix
Copy link
Contributor

@fabiofelix fabiofelix commented Sep 30, 2025

The following PR is about:

It unifies LoaderBase interface by:

  1. Replace load_data_from_file with load method
  2. Create a FileLoader class that inherits LoaderBase. Therefore, CSVLoader, ParquetLoader, and ShapefileLoader inherit this class
  3. Create DataFrameLoader and HuggingFaceLoader, which inherit LoaderBase, and moving specific codes from LoaderFactory.
  4. Refactor of LoaderFactory code, mainly build and load methods.
  5. Create/edit unit tests.
  6. Update the documentation and examples.

Now, it is possible to use HuggingFace (or dataframe) in a pipeline in the same way as file loaders, calling the factory method build.
Before, the user should've retrieved data from HuggingFace, saved it as a file, and used a file loader with the pipeline.


📚 Documentation preview 📚: https://UrbanMapper--80.org.readthedocs.build/en/80/

@fabiofelix fabiofelix requested a review from soniacq September 30, 2025 00:08
@fabiofelix fabiofelix self-assigned this Oct 1, 2025
@fabiofelix fabiofelix linked an issue Oct 1, 2025 that may be closed by this pull request
5 tasks
@fabiofelix fabiofelix added the enhancement New feature or request label Oct 1, 2025
Copy link
Contributor

@soniacq soniacq left a comment

Choose a reason for hiding this comment

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

Great PR! The main improvement is that Hugging Face data can now be used directly in the pipelines without converting it to a CSV first—this removes redundancy, which is excellent.
Just one minor fix: the docs/copy_of_examples folder should be removed, as it seems to have been committed by mistake. Apart from that, everything looks ready to merge.

@simonprovost
Copy link
Member

Great PR! The main improvement is that Hugging Face data can now be used directly in the pipelines without converting it to a CSV first—this removes redundancy, which is excellent. Just one minor fix: the docs/copy_of_examples folder should be removed, as it seems to have been committed by mistake. Apart from that, everything looks ready to merge.

If @fabiofelix did change the examples he needs to push the copy_of_examples in docs otherwise the "notebooks" are not forwarded to the documentation; just in case ✅

Let me have a Quick Look at the PR I have 5" before merging guys please

Cheers

@simonprovost simonprovost self-requested a review October 2, 2025 15:45
Copy link
Member

@simonprovost simonprovost left a comment

Choose a reason for hiding this comment

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

Amazing PR @fabiofelix ! Nothing of a big deal to change just to convince me this is the right way / expected, let me know!

Cheers

@fabiofelix fabiofelix force-pushed the feat/loader_build branch 2 times, most recently from 49a277a to a084d59 Compare October 2, 2025 21:05
Copy link
Member

@simonprovost simonprovost left a comment

Choose a reason for hiding this comment

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

Brilliant PR! Approving on my side of the table! Thanks for this great one @fabiofelix!

Next time, let's try to have a separate commit when you are changing things a little outside the scope of the PR, such as the viz. color palettes in the examples, so that this commit can have a description explaining why the change was made to avoid confusion, please.

I'll let @soniacq merge once she approves the PR 🫡

Cheers!

Copy link
Contributor

@soniacq soniacq left a comment

Choose a reason for hiding this comment

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

Thank you, Fabio, for addressing all the comments.

@soniacq soniacq merged commit 0d5653b into main Oct 6, 2025
11 checks passed
@soniacq soniacq deleted the feat/loader_build branch October 6, 2025 13:15
@soniacq soniacq restored the feat/loader_build branch October 6, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request loader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants