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.

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

Refactor for loops #294

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor for loops #294

wants to merge 2 commits into from

Conversation

burn2delete
Copy link
Contributor

Refactor some of the for loops into maps, including other code simplifications

@chungyau97
Copy link
Contributor

Results:
Document Loaders:

  1. Cheerio: Pass
    image

@HenryHengZJ
Copy link
Contributor

going to put this on hold first until we have less PRs

@deep-pipeline
Copy link

@burn2delete I was just having a glance at the PR code - because so many (20) files were altered - and I wonder if there are some maps happening now in your PR where, in fact, you want to keep the sequential dependence the slower for loop may provide.

In particular, and I may be getting this wrong because I was just glancing at the code, but you seem to be using map when assembling previous chat messages at one point into a context to be provided again. It could be argued that the sequence may not matter, but I think the stronger argument may be that the sequence COULD matter whereas the speedup in those instances is marginal? What do you think? I'm all for unrolling and parallelising, but if one simply does a search for all 'for' loops this doesn't filter if some of them are implicitly handling dependent sequences which shouldn't be parallelised and I don't know if you applied such checking given you did so many updates?

I realise this PR is almost a year old, so maybe things have overtaken this, but in case it matters for your own fork or if indeed this PR may yet get incorporated, I thought I should raise the issue before any merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants