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

Deprecate the current chunk_size in favour of batch_size for rg.log #2453

Closed
tomaarsen opened this issue Mar 2, 2023 · 0 comments · Fixed by #2455
Closed

Deprecate the current chunk_size in favour of batch_size for rg.log #2453

tomaarsen opened this issue Mar 2, 2023 · 0 comments · Fixed by #2455
Assignees
@tomaarsen
Labels
type: enhancement Indicates new feature requests
Milestone
v1.4.0

Comments

@tomaarsen
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I'd like to be able to log using the more intuitive batch_size parameter for batching my records. I believe that batch_size better aligns with the remainder of Argilla.

Describe the solution you'd like
I'd like to be able to perform

rg.log(records, name, batch_size=64)

Describe alternatives you've considered
Sticking with the current chunk_size.

Additional context
See #2434 (comment)
@tomaarsen tomaarsen added the type: enhancement Indicates new feature requests label Mar 2, 2023
@tomaarsen tomaarsen self-assigned this Mar 2, 2023
@tomaarsen tomaarsen mentioned this issue Mar 2, 2023
Merged
8 tasks
@frascuchon frascuchon added this to the v1.4.0 milestone Mar 2, 2023
frascuchon pushed a commit that referenced this issue Mar 6, 2023
@tomaarsen
feat: Deprecate chunk_size in favor of batch_size for rg.log (#…
3ebea76 
…2455)

Closes #2453

## Pull Request overview
* Deprecate `chunk_size` in favor of `batch_size` in `rg.log`:
  * Move `chunk_size` to the end of all related signatures.
  * Set `chunk_size` default to None and update the typing accordingly.
  * Introduce `batch_size` in the old position in the signature.
  * Update docstrings accordingly.
  * Introduce a `FutureWarning` if `chunk_size` is used.
* Introduce test showing that `rg.log(..., chunk_size=100)` indeed
throws a warning.
* Update a warning to no longer include a newline and a lot of spaces
(see first comment of this PR)

## Details
Note that this deprecation is non-breaking: Code that uses `chunk_size`
will still work, as `batch_size = chunk_size` after a FutureWarning is
given, if `chunk_size` is not `None`.

## Discussion
* Should I use a FutureWarning? Or a DeprecationWarning? Or a
PendingDeprecationWarning? The last two make sense, but they are
[ignored by
default](https://docs.python.org/3/library/warnings.html#default-warning-filter),
I'm afraid.
* Is the deprecation message in the format that we like?

---

**Type of change**
- [x] New feature (non-breaking change which adds functionality)

**How Has This Been Tested**

I introduced a test, and ran all tests.

**Checklist**

- [x] I have merged the original branch into my forked branch
- [x] I added relevant documentation
- [x] follows the style guidelines of this project
- [x] I did a self-review of my code
- [x] I made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works

---

- Tom Aarsen
@frascuchon frascuchon mentioned this issue Mar 8, 2023
Merged
frascuchon added a commit that referenced this issue Mar 9, 2023
@frascuchon
Release 1.4.0 (#2500)
cf75dd5 
# [1.4.0](v1.3.1...v1.4.0)
(2023-03-09)

### Features

* `configure_dataset` accepts a workspace as argument
([#2503](#2503))
([29c9ee3](29c9ee3)),
* Add `active_client` function to main argilla module
([#2387](#2387))
([4e623d4](4e623d4)),
closes [#2183](#2183)
* Add text2text support for prepare for training spark nlp
([#2466](#2466))
([21efb83](21efb83)),
closes [#2465](#2465)
[#2482](#2482)
* Allow passing workspace as client param for `rg.log` or `rg.load`
([#2425](#2425))
([b3b897a](b3b897a)),
closes [#2059](#2059)
* Bulk annotation improvement
([#2437](#2437))
([3fce915](3fce915)),
closes [#2264](#2264)
* Deprecate `chunk_size` in favor of `batch_size` for `rg.log`
([#2455](#2455))
([3ebea76](3ebea76)),
closes [#2453](#2453)
* Expose `batch_size` parameter for `rg.load`
([#2460](#2460))
([e25be3e](e25be3e)),
closes [#2454](#2454)
[#2434](#2434)
* Extend shortcuts to include alphabet for token classification
([#2339](#2339))
([4a92b35](4a92b35))


### Bug Fixes

* added flexible app redirect to docs page
([#2428](#2428))
([5600301](5600301)),
closes [#2377](#2377)
* added regex match to set workspace method
([#2427](#2427))
([d789fa1](d789fa1)),
closes [#2388]
* error when loading record with empty string query
([#2429](#2429))
([fc71c3b](fc71c3b)),
closes [#2400](#2400)
[#2303](#2303)
* Remove extra-action dropdown state after navigation
([#2479](#2479))
([9328994](9328994)),
closes [#2158](#2158)


### Documentation

* Add AutoTrain to readme
([7199780](7199780))
* Add migration to label schema section
([#2435](#2435))
([d57a1e5](d57a1e5)),
closes [#2003](#2003)
[#2003](#2003)
* Adds zero+few shot tutorial with SetFit
([#2409](#2409))
([6c679ad](6c679ad))
* Update readme with quickstart section and new links to guides
([#2333](#2333))
([91a77ad](91a77ad))


## As always, thanks to our amazing contributors!
- Documentation update: adding missing n (#2362) by @Gnonpi
- feat: Extend shortcuts to include alphabet for token classification
(#2339) by @cceyda
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tomaarsen tomaarsen

Labels
type: enhancement Indicates new feature requests
Projects
None yet
Milestone
v1.4.0
Development

Successfully merging a pull request may close this issue.

Deprecate chunk_size in favor of batch_size for rg.log tomaarsen/argilla
2 participants
@frascuchon @tomaarsen