Skip to content

Cpu buffer#1607

Open
thomasdeneux wants to merge 11 commits intovispy:mainfrom
thomasdeneux:CPU_buffer
Open

Cpu buffer#1607
thomasdeneux wants to merge 11 commits intovispy:mainfrom
thomasdeneux:CPU_buffer

Conversation

@thomasdeneux
Copy link
Copy Markdown
Contributor

Follow-up from previous PR #1538.

thomas bureau added 3 commits October 15, 2018 18:06
- stopped in the middle
- probably CPU buffer should be added to child DataBuffer rather than to
  Buffer, especially because DataBuffer.__init__ does not call
  Buffer.__init__
@thomasdeneux
Copy link
Copy Markdown
Contributor Author

did not pass the tests again... and i have difficulty reading the test reports to determine where is the problem

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Mar 22, 2019

Hm this may be something with conda/conda-forge on the python 3.7 environments. On the python 2.7 environments it looks like you need to fix the docstring to mention the new kwargs.

@kmuehlbauer
Copy link
Copy Markdown
Contributor

Blame me, will issue a hot fix instantly...

@kmuehlbauer
Copy link
Copy Markdown
Contributor

Hang on until this is ready: #1608

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Mar 22, 2019

@thomasdeneux could you try merging with master, pushing to this same branch, and seeing if the tests pass. You'll also need to update the docstring for the buffer to include your new kwarg.

@thomasdeneux
Copy link
Copy Markdown
Contributor Author

1/2 !!!
any help on what is missing?

@kmuehlbauer
Copy link
Copy Markdown
Contributor

@thomasdeneux Looking at the logs there is some flake8 errors.

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Mar 22, 2019

@thomasdeneux You can actually click on the "Details" from this pull request and click on the failing environments and see what is failing:

Running flake8... 
vispy/gloo/CPUData.py:64:20: F821 undefined name 'start'
vispy/gloo/CPUData.py:64:27: F821 undefined name 'stop'
vispy/gloo/CPUData.py:176:20: W292 no newline at end of file
Failed: flake8 failed
Running docstring test...
Testing failed (['flake'] failed, ['docs'] skipped) in 18.739 seconds
FAILURE

@thomasdeneux
Copy link
Copy Markdown
Contributor Author

got it, i fix it

@kmuehlbauer
Copy link
Copy Markdown
Contributor

@thomasdeneux There is a newline still missing at the eof.

@kmuehlbauer
Copy link
Copy Markdown
Contributor

@djhoese Could we have a test run just with flake8 which breaks instantly the whole suite?

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Mar 22, 2019

We could run it earlier in the relevant environments, but I'm not sure we can wait for one flake8 environment before running all other environments. Especially since travis gives us parallel processing for free (and building the flake environment would take minutes).

@kmuehlbauer
Copy link
Copy Markdown
Contributor

No, not wait, but break as soon as the flake env fails. If we take a Travis python env with just flake8 it would not take much time to fire up, or do I'm missing something.

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Mar 22, 2019

Ah good point. I'm not sure how to do that in travis.

@kmuehlbauer
Copy link
Copy Markdown
Contributor

Seems that is most wanted, but still not implemented: travis-ci/travis-ci#2062
I'll try to figure out something.

@kmuehlbauer
Copy link
Copy Markdown
Contributor

@thomasdeneux Green lights. Thanks for adding this!

Copy link
Copy Markdown
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Would it be possible to add some tests for this?

@thomasdeneux
Copy link
Copy Markdown
Contributor Author

thomasdeneux commented Mar 25, 2019

Ok, i will add some tests, probably next week

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Apr 28, 2020

@thomasdeneux Any chance you have time to merge with current master and we can see how the tests perform?

@thomasdeneux
Copy link
Copy Markdown
Contributor Author

oh i am busy these days... will try next week however

@rougier
Copy link
Copy Markdown
Contributor

rougier commented Feb 25, 2021

@thomasdeneux Any chance you can make the change?

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Apr 21, 2021

@thomasdeneux How busy are you a year after the last time I asked? 😜

@djhoese djhoese closed this Apr 28, 2021
@djhoese djhoese deleted the branch vispy:main April 28, 2021 19:00
@djhoese djhoese reopened this Apr 28, 2021
@djhoese djhoese closed this Apr 28, 2021
@djhoese djhoese deleted the branch vispy:main April 28, 2021 19:27
@djhoese djhoese reopened this Apr 28, 2021
@djhoese djhoese changed the base branch from master to main April 28, 2021 20:56
@thomasdeneux
Copy link
Copy Markdown
Contributor Author

@djhoese sorry i didn't know what to answer: i am really busy all the time and this CPU buffer improvement is a feature that is complete but which, at the end, i did not use; so it is not fresh in my mind, i expect it would take me 1~2 hours to go back into it and have the test passed; i would like to take this time, yet it cannot be this week. I was wondering if the best was not to forget about it, i.e. close the branch. Let's say if i find the time in the coming weeks i will still try to do it, otherwise it will remained closed...

@djhoese
Copy link
Copy Markdown
Member

djhoese commented Apr 29, 2021

Let's keep this open for now. I'll close it after a more thorough review and if I can't see an easy way forward. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants