GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix itertools chain by rng-dynamics · Pull Request #3788 · RustPython/RustPython

Conversation

@rng-dynamics

@rng-dynamics rng-dynamics commented Jun 14, 2022

Copy link
Copy Markdown
Contributor

Fix #3787

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
rng-dynamics added 3 commits June 11, 2022 00:01
@rng-dynamics
test: itertools.chain evaluate lazliy
a9d16e2
@rng-dynamics
feat: itertools.chain evaluate lazily
4c11720
@rng-dynamics
test: itertools.chain stop iteration on error
33bcc89
@rng-dynamics
rng-dynamics marked this pull request as draft June 14, 2022 08:27
fanninpm
fanninpm reviewed Jun 14, 2022
View reviewed changes
Comment thread vm/src/stdlib/itertools.rs
Comment on lines 79 to 99
fn next(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyIterReturn> {
loop {
let pos = zelf.cur_idx.load();
if pos >= zelf.iterables.len() {
break;
}
let cur_iter = if zelf.cached_iter.read().is_none() {
// We need to call "get_iter" outside of the lock.
let iter = zelf.iterables[pos].clone().get_iter(vm)?;
*zelf.cached_iter.write() = Some(iter.clone());
iter
} else if let Some(cached_iter) = (*zelf.cached_iter.read()).clone() {
cached_iter
} else {
// Someone changed cached iter to None since we checked.
continue;
};

// We need to call "next" outside of the lock.
match cur_iter.next(vm) {
Ok(PyIterReturn::Return(ok)) => return Ok(PyIterReturn::Return(ok)),
Ok(PyIterReturn::StopIteration(_)) => {
zelf.cur_idx.fetch_add(1);
*zelf.cached_iter.write() = None;
}
Err(err) => {
return Err(err);
let next = || {
let source = zelf.source.read().clone();
match source {
None => {
return Ok(PyIterReturn::StopIteration(None));
}
Some(source) => loop {
let active = zelf.active.read().clone();
match active {
None => match source.next(vm) {
Ok(PyIterReturn::Return(ok)) => {
*zelf.active.write() = Some(ok.get_iter(vm)?);
}
Ok(PyIterReturn::StopIteration(_)) => {
return Ok(PyIterReturn::StopIteration(None));
}
Err(err) => {
return Err(err);
}
},
Some(active) => match active.next(vm) {
Ok(PyIterReturn::Return(ok)) => {
return Ok(PyIterReturn::Return(ok));
}
Ok(PyIterReturn::StopIteration(_)) => {
*zelf.active.write() = None;
}
Err(err) => {
return Err(err);
}
},
}
},
}

@fanninpm fanninpm Jun 14, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

📎 "It looks like you're matching variants of an Option. Would you like help?" 📎

if let Some(source) = zelf.source.read().clone() {
    loop {
        if let Some(active) = zelf.active.read().clone() {
            match active.next(vm) {
                Ok(PyIterReturn::Return(ok)) => {
                    return Ok(PyIterReturn::Return(ok));
                }
                Ok(PyIterReturn::StopIteration(_)) => {
                    *zelf.active.write() = None;
                }
                Err(err) => {
                    return Err(err);
                }
            }
        } else {
            match source.next(vm) {
                Ok(PyIterReturn::Return(ok)) => {
                    *zelf.active.write() = Some(ok.get_iter(vm)?);
                }
                Ok(PyIterReturn::StopIteration(_)) => {
                    return Ok(PyIterReturn::StopIteration(None));
                }
                Err(err) => {
                    return Err(err);
                }
            }
        }
    }
} else {
    Ok(PyIterReturn::StopIteration(None))
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
youknowone
youknowone requested changes Jun 14, 2022
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you! The changes looks good.
Do you have more works to do? I wonder what made this marked as draft.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread vm/src/stdlib/itertools.rs Outdated
Comment on lines +68 to +70
match source {
None => {
return Ok(PyIterReturn::StopIteration(None));

@youknowone youknowone Jun 14, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
match source {
None => {
return Ok(PyIterReturn::StopIteration(None));
let source = if let Some(source) = source {
source
} else {
return Ok(PyIterReturn::StopIteration(None))
};

I have different suggestion to @fanninpm's one. Let's keep indent depth shallower

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@fanninpm fanninpm Jun 14, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@youknowone that suggestion has a few syntax errors in lines 68 and 71.

EDIT: I stand corrected regarding line 68.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
rng-dynamics added 3 commits June 14, 2022 18:57
@rng-dynamics
refactor
ec18a12
@rng-dynamics
Release source when PyIterReturn::StopIteration
76ef074
@rng-dynamics
fix tests
480f5f1
@rng-dynamics
rng-dynamics marked this pull request as ready for review June 14, 2022 18:57
@rng-dynamics

rng-dynamics commented Jun 14, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the suggestions. I tried to incorporate them and I tried to reduce the nesting. I also removed the closure which I didn't like that much. Let me know what you think. If you have more suggestions, I'd be happy to hear them.

Thank you! The changes looks good. Do you have more works to do? I wonder what made this marked as draft.

The main reason was the clippy test which failed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rng-dynamics rng-dynamics mentioned this pull request Jun 14, 2022
itertools.chain should evaluate lazily #3787
Closed
youknowone
youknowone approved these changes Jun 15, 2022
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good, thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@youknowone
youknowone merged commit cea5a6e into RustPython:main Jun 15, 2022
@youknowone

youknowone commented Jun 15, 2022

Copy link
Copy Markdown
Member

It fixed the ridiculously slow running of test_itertools.py, nice catch and fix!
Now the only slow running is tee
rng-dynamics reacted with heart emoji rng-dynamics reacted with rocket emoji
All reactions
  • ❤️ 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rng-dynamics
rng-dynamics deleted the fix-itertools-chain branch June 15, 2022 18:18
@rng-dynamics rng-dynamics mentioned this pull request Jun 19, 2022
Add itertools.chain.__setstate__ #3779
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fanninpm fanninpm fanninpm left review comments
@youknowone youknowone youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

itertools.chain should evaluate lazily

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@rng-dynamics @youknowone @fanninpm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.