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.

Sign up for GitHub

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

Jump to bottom

[ENHANCEMENT] [REFACTOR] optimise and refactor SDK ingestion methods #5107

Merged
merged 37 commits into from
Jul 4, 2024
Merged

[ENHANCEMENT] [REFACTOR] optimise and refactor SDK ingestion methods #5107

merged 37 commits into from
Jul 4, 2024

Conversation

burtenshaw
Copy link
Contributor

@burtenshaw burtenshaw commented Jun 25, 2024 •
edited
Loading

This PR refactors refactors the ingestion flow in DatasetRecords by implementing a new class and module IngestedRecordMapper

This PR supports mapping incoming columns/keys to dataset attributes in these two ways:

  • supports tuple values in the mapping parameter of the log method so that user can specify the two attributes as a tuple.
  • refactors in the _ingest_records methods so that mapping is performed once before the ingestion loop instead of during.

This PR also optimises the log method so that it takes less time and is easier to work with:

  • uses tqdm to log status
  • uses exception to show bad records
  • iterates over the map not the data
Screenshot 2024-06-26 at 09 09 11
  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

  • tests have been modified, deprecated, and updated to support changes in the ingestion flow

Checklist

  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • I confirm My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@burtenshaw burtenshaw changed the title [FEAT] map incoming columns to multiple dataset attributes [ENHANCEMENT] optimise SDK log method and support mapping incoming columns to multiple dataset attributes Jun 26, 2024
@nataliaElv
Copy link
Member

I'd rather have the records ingested and pushed in batches and have an easy way to identify those that threw an error, fix them and try to import those again.
Otherwise it can take ages until I see any records in my dataset.
Captura de pantalla 2024-06-26 a las 9 23 42

davidberenstein1957
davidberenstein1957 reviewed Jun 26, 2024
View reviewed changes
Copy link
Member

@davidberenstein1957 davidberenstein1957 left a comment

Choose a reason for hiding this comment

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

Left some initial comments.

argilla/src/argilla/records/_dataset_records.py Outdated Show resolved Hide resolved
argilla/src/argilla/records/_dataset_records.py Outdated Show resolved Hide resolved
argilla/src/argilla/records/_dataset_records.py Outdated Show resolved Hide resolved
argilla/src/argilla/records/_dataset_records.py Outdated Show resolved Hide resolved
frascuchon and others added 7 commits June 27, 2024 10:43
@frascuchon
[REFACTOR] Avoid autofetch when accessing settings (#5112)
db08548 
Reviewing and improving records.log

Instead of: 

<img width="1335" alt="Captura de pantalla 2024-06-26 a las 12 48 14"
src="https://github.com/argilla-io/argilla/assets/2518789/02283f4c-fe6a-464f-96b3-36853e6c7622">

for 50 records, records.log can log 1000:

<img width="870" alt="Captura de pantalla 2024-06-26 a las 12 48 57"
src="https://github.com/argilla-io/argilla/assets/2518789/d20f0469-0b33-427e-aa12-b4b7e1d40cd1">
@burtenshaw
Merge branch 'spike/mapping-to-tuple' of https://github.com/argilla-i…
5f06e20 
…o/argilla into spike/mapping-to-tuple
@burtenshaw
enhancement: use just one progress bar
05df51a
@burtenshaw
chore: update typing of mapping
863dde2
@burtenshaw
fix: move render mapping into infer record method
bf9e864
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
07aa249 
for more information, see https://pre-commit.ci
@burtenshaw
fix: align add records parameters with render function
8a6d484
@frascuchon frascuchon mentioned this pull request Jun 28, 2024
Closed
@burtenshaw burtenshaw marked this pull request as draft June 28, 2024 12:33
@burtenshaw burtenshaw marked this pull request as ready for review July 2, 2024 15:21
burtenshaw and others added 11 commits July 3, 2024 13:43
@burtenshaw
docs: typo in docs
3eb8c0d
@burtenshaw
feat: improve record switch in ingest method
b10fbe8
@burtenshaw
feat: refactor id mapping away from dict to type
db27e1b
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
716dfa8 
for more information, see https://pre-commit.ci
@burtenshaw
feat: use class methods for type and parameter values
6a7e9eb
@burtenshaw
Merge branch 'spike/mapping-to-tuple' of https://github.com/argilla-i…
ca1a394 
…o/argilla into spike/mapping-to-tuple
@frascuchon
[REFACTOR] generate default mapping and extends it with custom mappin…
40ff5b6 
…g dict (#5151)

<!-- Please include a summary of the changes and the related issue.
Please also include relevant motivation and context. List any
dependencies that are required for this change. -->

Closes #<issue_number>

**Type of change**
<!-- Please delete options that are not relevant. Remember to title the
PR according to the type of change -->

- Refactor (change restructuring the codebase without changing
functionality)

**How Has This Been Tested**
<!-- Please add some reference about how your feature has been tested.
-->

**Checklist**
<!-- Please go over the list and make sure you've taken everything into
account -->

- I added relevant documentation
- follows the style guidelines of this project
- I did a self-review of my code
- I made corresponding changes to the documentation
- I confirm My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature
works
- I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)
@burtenshaw
refactor: migrate mapper into module from file
667ad54
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
94988eb 
for more information, see https://pre-commit.ci
@burtenshaw
fix: raise error not warn when mapped attribute is unknown
2769793
@burtenshaw
Merge branch 'spike/mapping-to-tuple' of https://github.com/argilla-i…
d83cf7a 
…o/argilla into spike/mapping-to-tuple
@frascuchon frascuchon merged commit 8c808f5 into develop Jul 4, 2024
7 checks passed
@frascuchon frascuchon deleted the spike/mapping-to-tuple branch July 4, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidberenstein1957 davidberenstein1957 davidberenstein1957 left review comments

@frascuchon frascuchon frascuchon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@burtenshaw @nataliaElv @frascuchon @davidberenstein1957