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

feat(commands): add lossy flag when creating annotations #289

Merged
merged 6 commits into from
Jul 24, 2024
Merged

feat(commands): add lossy flag when creating annotations #289

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
62ab5c1
feat(commands): add lossy flag when creating annotations
joe-prosser Jul 24, 2024
4345cef
update changelog
joe-prosser Jul 24, 2024
4198ccf
tweak resuming message
joe-prosser Jul 24, 2024
9c5e67a
add progress bar
joe-prosser Jul 24, 2024
9bb5df2
post review fix up
joe-prosser Jul 24, 2024
8acf879
clear annotations to upload when resuming
joe-prosser Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
- Add `config parse-from-url` command for parsing configuration from a URL
- Add ability to download attachments for comments
- Increase default http timeout to 120s
- Add `--lossy` flag when creating annotations

# v0.28.0
- Add general fields to `create datasets`
Expand Down
51 changes: 46 additions & 5 deletions cli/src/commands/create/annotations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::progress::{Options as ProgressOptions, Progress};
use crate::{
print_error_as_warning,
progress::{Options as ProgressOptions, Progress},
};
use anyhow::{Context, Result};
use colored::Colorize;
use log::info;
Expand Down Expand Up @@ -47,6 +50,10 @@ pub struct CreateAnnotationsArgs {
#[structopt(long = "batch-size", default_value = "128")]
/// Number of comments to batch in a single request.
batch_size: usize,

#[structopt(long)]
/// Whether to attempt to resume processing on error
lossy: bool,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

lossy isn't very descriptive. How about resume-on-error?


pub fn create(client: &Client, args: &CreateAnnotationsArgs, pool: &mut Pool) -> Result<()> {
Expand Down Expand Up @@ -95,6 +102,7 @@ pub fn create(client: &Client, args: &CreateAnnotationsArgs, pool: &mut Pool) ->
args.use_moon_forms,
args.batch_size,
pool,
args.lossy,
)?;
if let Some(mut progress) = progress {
progress.done();
Expand All @@ -117,6 +125,7 @@ pub fn create(client: &Client, args: &CreateAnnotationsArgs, pool: &mut Pool) ->
args.use_moon_forms,
args.batch_size,
pool,
args.lossy,
)?;
statistics
}
Expand All @@ -131,8 +140,10 @@ pub fn create(client: &Client, args: &CreateAnnotationsArgs, pool: &mut Pool) ->

pub trait AnnotationStatistic {
fn add_annotation(&self);
fn add_failed_annotation(&self);
}

#[allow(clippy::too_many_arguments)]
pub fn upload_batch_of_annotations(
annotations_to_upload: &mut Vec<NewAnnotation>,
client: &Client,
Expand All @@ -141,6 +152,7 @@ pub fn upload_batch_of_annotations(
dataset_name: &DatasetFullName,
use_moon_forms: bool,
pool: &mut Pool,
lossy: bool,
) -> Result<()> {
let (error_sender, error_receiver) = channel();

Expand Down Expand Up @@ -182,15 +194,21 @@ pub fn upload_batch_of_annotations(

if let Err(error) = result {
error_sender.send(error).expect("Could not send error");
statistics.add_failed_annotation();
} else {
statistics.add_annotation();
}

statistics.add_annotation();
});
})
});

if let Ok(error) = error_receiver.try_recv() {
Err(error)
if lossy {
print_error_as_warning(&error);
Ok(())
} else {
Err(error)
}
} else {
annotations_to_upload.clear();
Ok(())
Expand All @@ -207,6 +225,7 @@ fn upload_annotations_from_reader(
use_moon_forms: bool,
batch_size: usize,
pool: &mut Pool,
lossy: bool,
) -> Result<()> {
let mut annotations_to_upload = Vec::new();

Expand All @@ -224,6 +243,7 @@ fn upload_annotations_from_reader(
dataset_name,
use_moon_forms,
pool,
lossy,
)?;
}
}
Expand All @@ -238,6 +258,7 @@ fn upload_annotations_from_reader(
dataset_name,
use_moon_forms,
pool,
lossy,
)?;
}

Expand Down Expand Up @@ -307,19 +328,24 @@ fn read_annotations_iter<'a>(
pub struct Statistics {
bytes_read: AtomicUsize,
annotations: AtomicUsize,
failed_annotations: AtomicUsize,
}

impl AnnotationStatistic for Statistics {
fn add_annotation(&self) {
self.annotations.fetch_add(1, Ordering::SeqCst);
}
fn add_failed_annotation(&self) {
self.failed_annotations.fetch_add(1, Ordering::SeqCst);
}
}

impl Statistics {
fn new() -> Self {
Self {
bytes_read: AtomicUsize::new(0),
annotations: AtomicUsize::new(0),
failed_annotations: AtomicUsize::new(0),
}
}

Expand All @@ -337,14 +363,29 @@ impl Statistics {
pub fn num_annotations(&self) -> usize {
self.annotations.load(Ordering::SeqCst)
}

#[inline]
pub fn num_failed_annotations(&self) -> usize {
self.failed_annotations.load(Ordering::SeqCst)
}
}

fn basic_statistics(statistics: &Statistics) -> (u64, String) {
let bytes_read = statistics.bytes_read();
let num_annotations = statistics.num_annotations();
let num_failed_annotations = statistics.num_failed_annotations();
(
bytes_read as u64,
format!("{} {}", num_annotations, "annotations".dimmed(),),
format!(
"{} {}{}",
num_annotations,
"annotations".dimmed(),
if num_failed_annotations > 0 {
format!(" {} {}", num_failed_annotations, "skipped".dimmed())
} else {
"".to_string()
}
),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The nested format! is a bit cheeky. How about:

let failed_annotations_string = if num_failed_annotations > 0 {
    format!(" {num_failed_annotations} {}", "skipped".dimmed())
} else {
    String::new()
};

Similarly below.

}

Expand Down
35 changes: 33 additions & 2 deletions cli/src/commands/create/comments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ pub struct CreateCommentsArgs {
#[structopt(short = "y", long = "yes")]
/// Consent to ai unit charge. Suppresses confirmation prompt.
yes: bool,

#[structopt(long)]
/// Whether to attempt to resume processing on error
lossy: bool,
}

pub fn create(client: &Client, args: &CreateCommentsArgs, pool: &mut Pool) -> Result<()> {
Expand Down Expand Up @@ -152,6 +156,7 @@ pub fn create(client: &Client, args: &CreateCommentsArgs, pool: &mut Pool) -> Re
args.use_moon_forms,
args.no_charge,
pool,
args.lossy,
)?;
if let Some(mut progress) = progress {
progress.done();
Expand Down Expand Up @@ -180,6 +185,7 @@ pub fn create(client: &Client, args: &CreateCommentsArgs, pool: &mut Pool) -> Re
args.use_moon_forms,
args.no_charge,
pool,
args.lossy,
)?;
statistics
}
Expand Down Expand Up @@ -330,6 +336,7 @@ fn upload_comments_from_reader(
use_moon_forms: bool,
no_charge: bool,
pool: &mut Pool,
lossy: bool,
) -> Result<()> {
assert!(batch_size > 0);

Expand Down Expand Up @@ -415,6 +422,7 @@ fn upload_comments_from_reader(
dataset_name,
use_moon_forms,
pool,
lossy,
)?;
}
}
Expand Down Expand Up @@ -442,6 +450,7 @@ fn upload_comments_from_reader(
dataset_name,
use_moon_forms,
pool,
lossy,
)?;
}
}
Expand All @@ -464,12 +473,16 @@ pub struct Statistics {
updated: AtomicUsize,
unchanged: AtomicUsize,
annotations: AtomicUsize,
failed_annotations: AtomicUsize,
}

impl AnnotationStatistic for Statistics {
fn add_annotation(&self) {
self.annotations.fetch_add(1, Ordering::SeqCst);
}
fn add_failed_annotation(&self) {
self.failed_annotations.fetch_add(1, Ordering::SeqCst);
}
}

impl Statistics {
Expand All @@ -481,6 +494,7 @@ impl Statistics {
updated: AtomicUsize::new(0),
unchanged: AtomicUsize::new(0),
annotations: AtomicUsize::new(0),
failed_annotations: AtomicUsize::new(0),
}
}

Expand Down Expand Up @@ -526,6 +540,11 @@ impl Statistics {
fn num_annotations(&self) -> usize {
self.annotations.load(Ordering::SeqCst)
}

#[inline]
fn num_failed_annotations(&self) -> usize {
self.failed_annotations.load(Ordering::SeqCst)
}
}

/// Detailed statistics - only make sense if using --overwrite (i.e. exclusively sync endpoint)
Expand All @@ -537,10 +556,11 @@ fn detailed_statistics(statistics: &Statistics) -> (u64, String) {
let num_updated = statistics.num_updated();
let num_unchanged = statistics.num_unchanged();
let num_annotations = statistics.num_annotations();
let num_failed_annotations = statistics.num_failed_annotations();
(
bytes_read as u64,
format!(
"{} {}: {} {} {} {} {} {} [{} {}]",
"{} {}: {} {} {} {} {} {} [{} {}{}]",
num_uploaded.to_string().bold(),
"comments".dimmed(),
num_new,
Expand All @@ -551,6 +571,11 @@ fn detailed_statistics(statistics: &Statistics) -> (u64, String) {
"nop".dimmed(),
num_annotations,
"annotations".dimmed(),
if num_failed_annotations > 0 {
format!(" {} {}", num_failed_annotations, "skipped".dimmed())
} else {
"".to_string()
}
),
)
}
Expand All @@ -560,14 +585,20 @@ fn basic_statistics(statistics: &Statistics) -> (u64, String) {
let bytes_read = statistics.bytes_read();
let num_uploaded = statistics.num_uploaded();
let num_annotations = statistics.num_annotations();
let num_failed_annotations = statistics.num_failed_annotations();
(
bytes_read as u64,
format!(
"{} {} [{} {}]",
"{} {} [{} {}{}]",
num_uploaded.to_string().bold(),
"comments".dimmed(),
num_annotations,
"annotations".dimmed(),
if num_failed_annotations > 0 {
format!(" {} {}", num_failed_annotations, "skipped".dimmed())
} else {
"".to_string()
}
),
)
}
Expand Down
19 changes: 15 additions & 4 deletions cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,15 +203,26 @@ fn find_configuration(args: &Args) -> Result<PathBuf> {
Ok(config_path)
}

pub fn print_error_as_warning(error: &anyhow::Error) {
warn!("An error occurred. Resuming...");
for cause in error.chain() {
warn!(" |- {}", cause);
}
}

pub fn print_error(error: &anyhow::Error) {
error!("An error occurred:");
for cause in error.chain() {
error!(" |- {}", cause);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need two functions. How about just

pub fn print_error(error: &anyhow::Error) {
    for cause in error.chain() {
        error!(" |- {}", cause);
    }
}

and leave the call to error!()before it in main() below. You can add the call to warn!() in upload_batch_of_annotations(), but I'm not sure logging in that case is necessary.


fn main() {
let args = Args::from_args();
utils::init_env_logger(args.verbose);

if let Err(error) = run(args) {
error!("An error occurred:");
for cause in error.chain() {
error!(" |- {}", cause);
}
print_error(&error);

#[cfg(feature = "backtrace")]
{
Expand Down
Loading