Cannot send to destroyed tchannel

Error: cannot send() to destroyed tchannel
    at TChannel.send [as request] (/home/raynos/uber/autobahn/node_modules/tchannel/index.js:286:15)
    at TChannel.send (/home/raynos/uber/autobahn/node_modules/tchannel/index.js:278:20)
    at new PingSender (/home/raynos/uber/autobahn/node_modules/ringpop/lib/swim.js:84:23)
    at RingPop.sendPing (/home/raynos/uber/autobahn/node_modules/ringpop/index.js:695:12)
    at RingPop.protocolPingReq (/home/raynos/uber/autobahn/node_modules/ringpop/index.js:457:10)
    at RingPopTChannel.protocolPingReq (/home/raynos/uber/autobahn/node_modules/ringpop/lib/tchannel.js:183:18)
    at runHandler (/home/raynos/uber/autobahn/node_modules/tchannel/index.js:764:14)
    at process._tickDomainCallback (node.js:459:13)

It looks like if we closed ringpop whilst a ping is mid flight it does not check whether its destroyed in the protocolPingReq method of ringpop.

ack. thank you for your submission ;)


Native Strategy: Samsung Streaming Media Player

📺 What
Adds the samsung maple media player from TAL as an additional configurable media player option for nativestrategy playback.

Tickets: IPLAYERTVV1-9132

🛠 How
Refactors legacy TAL code into bigscreen-player's media player format.

Closing as implemented in a later PR (#171)


Cryptic ArgumentError lets ERAlchemy fail to load sqlite db

Some time ago, I managed to generate a nice ER diagram from a previous version of my database. Howerver today, running eralchemy on my sqlite database
$ file lexirumah.sqlite
lexirumah.sqlite: SQLite 3.x database

now fails with a value error which does not make obvious what the problem is. I have checked with the current version of eralchemy from pip, with the most recent checkout of the master branch and with tag v1.0.15, and in all cases I get
$ eralchemy -i "sqlite://lexirumah.sqlite" -o erd_from_sqlite.svg
Traceback (most recent call last):
  File "/vol/home/kaipingga/.local/bin/eralchemy", line 11, in <module>
    load_entry_point('ERAlchemy', 'console_scripts', 'eralchemy')()
  File "/vol/home/kaipingga/devel/eralchemy/eralchemy/main.py", line 33, in cli
    render_er(args.i, args.o, exclude=args.x, schema=args.s)
  File "/vol/home/kaipingga/devel/eralchemy/eralchemy/main.py", line 179, in render_er
    tables, relationships = all_to_intermediary(input, schema=schema)
  File "/vol/home/kaipingga/devel/eralchemy/eralchemy/main.py", line 130, in all_to_intermediary
    raise ValueError(msg)
ValueError: Cannot process filename_or_input str

Running pdb on the problem shows that the problem is that database_to_intermediary finds some problem somewhere in my DB and fails with an ArgumentError, which leads the calling function to assume that it got passed something that was not a DB URI as filename_or_input (which in this case is the wrong conclusion).
(Pdb) database_to_intermediary(filename_or_input, schema=None)
*** sqlalchemy.exc.ArgumentError: WARNING: when configuring property 'value' on Mapper|value_data|value_data, column 'value' conflicts with property '<RelationshipProperty at 0x7fb3554abd48; value>'. To resolve this, map the column to the class under a different name in the 'properties' dictionary.  Or, to remove all awareness of the column entirely (including its availability as a foreign key), use the 'include_properties' or 'exclude_properties' mapper arguments to control specifically which table columns get mapped.

I have no idea what this means. I have attached the output of sqlite3 lexirumah.sqlite .dump | grep -v INSERT, in case looking at the table schema helps finding the issue.
db_schema.txt

The complete traceback below database_to_intermediary is as follows.
  File "/vol/home/kaipingga/devel/eralchemy/eralchemy/sqla.py", line 77, in database_to_intermediary
    Base.prepare(engine, reflect=True)
  File "/vol/home/kaipingga/.local/lib/python3.5/site-packages/sqlalchemy/ext/automap.py", line 799, in prepare
    map_config.map()
  File "/vol/home/kaipingga/.local/lib/python3.5/site-packages/sqlalchemy/ext/declarative/base.py", line 643, in map
    return super(_DeferredMapperConfig, self).map()
  File "/vol/home/kaipingga/.local/lib/python3.5/site-packages/sqlalchemy/ext/declarative/base.py", line 576, in map
    **self.mapper_args
  File "<string>", line 2, in mapper
  File "/vol/home/kaipingga/.local/lib/python3.5/site-packages/sqlalchemy/orm/mapper.py", line 692, in __init__
    self._configure_properties()
  File "/vol/home/kaipingga/.local/lib/python3.5/site-packages/sqlalchemy/orm/mapper.py", line 1417, in _configure_properties
    setparent=True)
  File "/vol/home/kaipingga/.local/lib/python3.5/site-packages/sqlalchemy/orm/mapper.py", line 1621, in _configure_property
    prop = self._property_from_column(key, prop)
  File "/vol/home/kaipingga/.local/lib/python3.5/site-packages/sqlalchemy/orm/mapper.py", line 1794, in _property_from_column
    (key, self, column.key, prop))
sqlalchemy.exc.ArgumentError: WARNING: when configuring property 'value' on Mapper|value_data|value_data, column 'value' conflicts with property '<RelationshipProperty at 0x7f0d3c0edbc8; value>'. To resolve this, map the column to the class under a different name in the 'properties' dictionary.  Or, to remove all awareness of the column entirely (including its availability as a foreign key), use the 'include_properties' or 'exclude_properties' mapper arguments to control specifically which table columns get mapped.

Hi @Anaphory,
Did you find a workaround? I am facing the same issue.

I assume you have to add a third / to your command like this:
eralchemy -i "sqlite:///lexirumah.sqlite" -o erd_from_sqlite.svg

Wow, yes! That looks like a very correct assessment of the problem. Can that be caught and the user be given a hint?


Class attribute directive is ignored on rendered mock directive

I have the following HTML:

...

I'm trying to unit test if the extra-class gets added to the div when someCondition is true. However for this test i've changed it to: [class.extra-class]="true"
the test looks like this:
const preview = ngMocks.findInstance(fixture.debugElement, CdkDragPreview) as MockedDirective<CdkDragPreview>;
preview.__render();

// don't know how to get the classes from the preview, so do a normal query
expect(fixture.debugElement.query(By.css('.dragdrop-preview')).nativeElement).toHaveClass('extra-class');

I get the following error:
Error: Expected
...
 to have class 'extra-class'.

I expected it to have the extra-class class. Am I correct in my assumption that this class attribute directive is ignored when rendering and if so, could this be added, or am I testing the wrong way?

Hi @JesseZomer,
thanks for the report, might you provide a min test? I'll take a look tonight.

Sure, I've made a repo here: https://github.com/JesseZomer/ngmocksIssue176
If you run ng test the test described above will fail

Hi @JesseZomer,
after changing things that cause template change you need to call detect changes:
    preview.__render();
    fixture.detectChanges(); // <-- add this.

Hello @satanTime, thanks for the reply.
I've tried it with fixture.detectChanges() but that makes no difference.
I thought because the query obviously finds an element the fixture.detectChanges makes no difference, so I removed it again.

when I added it in https://github.com/JesseZomer/ngmocksIssue176 there's no failing test anymore. Might you improve the example to show the failure with fixture.detectChanges()?

That's weird. That made me look at the differences between the reproduction test and the real testcase.
In the real situation the component has a changedetectionStrategy of OnPush. Because I wasn't changing any @input fields, the class directive probably never gets reapplied.
When I add
        preview.__render();
        component.input= { ...component.input };
        fixture.detectChanges();

it works 🎉
I probably didn't think of this, because something did get rendered so the HTML had changed.
Thank you for taking the time to help me fix this issue 👏

Glad to hear!


Update to newer QPU

Minor updates to the Summary section.
Note that the latest version seems to add attachments metadata: I opened in the IDE and it doesn't cause a problem but I can manually delete if anyone is worried.

The circleci test failure is due to "API token not defined", running on my PR and I think it used to run only on master, so I think it's just not getting the usual access to a token.


Vue-cli webpack and server backend dev hot-reaload

Hello,
I am trying to understand how to specify the __hmr url.
Because when I include my file.js, for example http: // localhost: 8080 / app.js
In my page the hmr takes the address of my backend server http: // localhost: 2000 / __ hmr ... and refuses to load
Do you have a solution so that I can modify my configuration, in order to be able to reload my page automatically during my development
Thank you

Hi @M-kasinski. I'm guessing you're using one of the vuejs-templates, please open issue there.
Webpack's hmr should work out of the box.
I'm not really sure what's the issue but because you're mentioning different backend server I'll point you here: http://vuejs-templates.github.io/webpack/proxy.html


Add support for PHP Pear and unify PECL with it

You may want to hold off on this PR pending the outcome of package-url/purl-spec#300. I think this way makes sense, but there are arguments to be made for doing it either way.

Waiting on https://github.com/package-url/purl-spec/pull/300 to resolve


Add 'continous-integration.yml'

Hi, thanks for the commit!
1- this is amazing
2- this fails in macos and windows on the same issue. Do you have any ideas as to how to fix that?
3- I think this is the truth being reported by CI, should we make it work in MacOS and Windows and merge this first as a litmus test?
thoughts?

I have a better version here #3 in case you wanted to see


Vickel: Watch au.makemyassignments.co.nz

Vickel requests the watch of the watch_keyword au\.makemyassignments\.co\.nz. See the MS search here and the Stack Exchange search in text, in URLs, and in code.

au\.makemyassignments\.co\.nz has been seen in 1 true positive, 0 false positives, and 0 NAAs.

Rejected by Nick in Charcoal HQ. Reason: 'superseded by 10699'


Signal Quality Indicator

With the updated Noise Detector code, it should be fairly easy to have a long running signal quality indicator component.
This could be a component mounted in index that will appear whenever noise is detected in an epoch with a graphic indicating at which point on the head the noise appears.
OR
this could be a transition screen when starting the tutorial (like in the calm app)
Perhaps a mixture of the transition screen (for the tutorial) and a little widget (for sandbox and BCI) would be best

Built the NoiseIndicator, which pops up on screen with the head diagram and the electrodes in which noise is detected indicated with little red error icons


Add draw_blurred_rounded_rect_in (intended for box shadows)

This avoids needing a clip layer for a rounded rectangle which doesn't need to draw the inner parts (e.g. a transparent background with a box shadow).
The previous test image was oversized, so this also fixes that (and makes any future such image fail the tests as expected). I imagine that the author of that test manually copied the value into the place, not realising the size of the image.

I was intending to do a proper review, but my comments would be:

looks good!
I assume there is some efficiency advantage to this over a short-lived layer?
Because it looks like the hard (well, more just awkward) bit (creating the clip shape) is still left to user code.
It would definitely nice to get some kind of helper for this "rounded rect donut" shape

Finally a question:

Would this work for inset shadows? I guess it would and I could just use a regular rect as the clip?

thanks

Yes, I believe so - it reduces the number of segments which need to be processed, and avoids some copies of data in fine.

Yeah - I didn't want to look into the exact semantics of box shadow's shapes

I did originally have this utility, but it made some really ugly looking boundaries. I don't think that such a niche shape is a meaningful addition to the imaging model

Yeah, just using the rounded rectangle as the drawing shape should work, if inset shadows are what I think they are.

Yes, I believe so - it reduces the number of segments which need to be processed, and avoids some copies of data in fine.

Awesome.

Yeah, just using the rounded rectangle as the drawing shape should work, if inset shadows are what I think they are.

Excellent, I'll try this soon :)

I did originally have this utility, but it made some really ugly looking boundaries. I don't think that such a niche shape is a meaningful addition to the imaging model
Yeah - I didn't want to look into the exact semantics of box shadow's shapes

Perhaps instead of some generic/composable utility in kurbo there ought to some super-simple box_shadow method on Scene that just generates shape from rect as in your example code above and then defers to draw_blurred_rounded_rect_in? Which we could add once we've nailed down exactly what the clip path should be for a given rect/shadow I guess.

Again, I don't really see the advantage of that - that was part of my original implementation, but I removed it for simplicity.
As you say, that wouldn't work for inset shadows; I don't really think it is very useful.

My motivation for such a feature is that:
let kernel_size = 2.5 * std_dev;
let shape = BezPath::from_iter(
    rect.inflate(kernel_size, kernel_size)
        .path_elements(0.1)
        .chain(
            RoundedRect::from_rect(rect, radius)
                .to_path(0.1)
                .reverse_subpaths(),
        ),
);

is not particularly simple or obvious to construct to those without knowledge of vector graphics who just want to a draw a box shadow with sensible defaults. It also requires quite a strong knowledge of kurbo to get it that simple (consider the code we currently have in Blitz for this).
As the shape can be derived from the rect and shadow parameters, it would be convenient if there was a method that encapsulated that knowledge. Then the API for consumers of Vello would be the same simpler API as that of draw_blurred_rounded_rect. For inset shadows we'd need an additional method.
But eh, it's not the most important thing in the world. As-is, this seems like a nice improvement on what we had before.


Add support for cbar_mode='edge'

Closes #16
@nathanieltarshish I wanted it for a plot I was making, so I went ahead and implemented it!  If you feel inclined, it would be great if you could give this a review.

Great, thanks for adding this! Looks good to me.

Thanks @nathanieltarshish!  Merging now.


Unable to deploy with near-bos-cli

Affected Portal:
Select one: DevHub ( might affect all )
Description:
The DevHUB deployment pipeline currently fails with the following error from bos-cli
Error:
Your console command:
bos components deploy devhub.near sign-as devhub.near network-config mainnet sign-with-plaintext-private-key --signer-public-key ed25519:3eLvVvLmB4gfJaZWYCwNLQXHhkxzJNvrdX5V8YCz1WMV --signer-private-key *** send
   0: ERROR: Social DB requires more storage deposit, but we cannot cover it when signing transaction with a Function Call only access key

Since we saw that it is possible to deploy by calling set directly on social.near using near-cli-rs as done in https://github.com/NEAR-DevHub/neardevhub-bos/pull/894, we know that the error message from bos-cli is wrong. bos-cli is estimating wrong when it comes to the storage deposit required.
Also 10 NEAR was deposited for devhub to increase storage ( see transactions 3bnAqyNbtnkq7D67t6kGoTEFQG9gyEvYr71GEzQA9oaS,  41hZN6kWEdYgr6A3hHxhsavhJRJoQLUPsfzbfNKTzfGo,
233YEPkmLKcFrLByHv1bXWTYT1yndAGQJwrbdHmxDL7j,
ATbWDMxPHuNQ8gdESBHWw14qUCddEEXsw8jpb4Eaf6tx and DL6pu1tugLmAuHkx6ceHhCSzntWYbf9jNAn7CGL6BLWH
) with the result of increased storage, as we can see when running
 bos social-db prepaid-storage view-balance devhub.near network-config mainnet now
storage balance for <devhub.near>:
 available:        1.8 MB   (17.72 NEAR [<PHONE_NUMBER>0000000000000014 yoctoNEAR])
 total:            2.7 MB   (27.11 NEAR [<PHONE_NUMBER>0000000000000014 yoctoNEAR])

Still bos components deploy fails with the error message about insufficient deposit for storage.
User Type:
Specify:  Other
Reproduction Steps
Any deployment with bos cli
Expected behavior
BOS cli should estimate storage deposit requirements correctly, or not estimate at all and let the deployment transaction fail instead.
Visual Reference
If applicable, add screenshots or video recording to help explain your problem.
Priority
Select One: High (P1) – Unexpected error or behavior ( All deployments are affected, it is possible to work around it by calling set directly on the social.near contract without using bos cli ).
Environment Details
N/A
Additional Context
N/A

Turns out to be the that the public key that we use for deployment does not have write permission in socialdb.

Will be fixed by https://github.com/bos-cli-rs/near-socialdb-client-rs/pull/11


Documentation for keycloak 17+

Hi, the path of the plugin for Keycloak version 17 and newer is now /opt/keycloak/providers/keycloak-to-rabbit-3.0.jar.
USAGE:

Download the latest jar or build from source: mvn clean install
Copy jar into your Keycloak

Keycloak version 17+ /opt/keycloak/providers/keycloak-to-rabbit-3.0.jar
Keycloak version 16 and older /opt/jboss/keycloak/standalone/deployments/keycloak-to-rabbit-3.0.jar

Configure as described below (option 1 or 2 or 3)
Restart the Keycloak server
Enable logging in Keycloak UI by adding keycloak-to-rabbitmq
Manage > Events > Config > Events Config > Event Listeners

Hi! Where do we find a complete list of valid KK.EVENT.ADMIN..SUCCESS.*** events to bind to?

@durop Thank you, I have now updated the documentation as you suggested.


Vehicle.commands.download() timeout when called multiple times on a telemetry link brokered by MAVProxy

When calling Vehicle.commands.download() and Vehicle.commands.wait_ready() repeatedly through MAVProxy, the wait_ready() always fail after a number of trials:
  File "/usr/local/lib/python2.7/dist-packages/dronekit/__init__.py", line 2650, in wait_ready
    return self._vehicle.wait_ready('commands', **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/dronekit/__init__.py", line 2182, in wait_ready
    timeout)
dronekit.APIException: wait_ready experienced a timeout after 30 seconds.

The above error is guaranteed to be thrown when running the following integration test:
https://gist.github.com/tribbloid/13d1bb022c0b681a9ee4a65f1067376e
I don't know if its a DK issue or MAVProxy issue. So I'll just submit a failed test to both projects. Please resolve it if DK is not the culprit.

I can verify that I am having this same problem. If after calling cmds.download() in DroneKit, I go to MavProxy and see "No waypoint load started" as a response to that requestion, then I issue a mp command "wp list" DroneKit unblocks, receives the download and continues. This isn't a problem with a direct connection or SITL.
This is causing me to be super stuck at the moment with no workaround.

I get this issue as well. In addition, sometimes calling connect('connection_string', wait_ready=True) causes this issue as well.
@peterbarker Any help here would be appreciated. I am able to connect to Mission Planner from MAVProxy, but I cannot run a drone kit connection. I can provide any information you need for debugging the issue.
Thank you for the help!

Some additional information that we discovered in our application:
On SITL or direct serial connection, this wasn't a problem. On a radio connection, this was a huge problem. We also found that if we had a ground station (Mission Planner) using the same connection as DroneKit, we could get it to work. It was assumed that something in the exchange of data from Mission Planner through DroneKit was satisfying Wait Ready statements. Also, while the documentation for Vehicle.commands.wait_ready and Vehicle.wait_ready implies that the first just waits for a mission to be downloaded, it appears they both hang in the identical way. Also, the docs suggest doing a Vehicle.commands.wait_ready after a vehicle.commands.download, however a review of the code on GitHub suggests that the wait_ready has been added to the downloads function, and doesn't need to be added again.

@custeve If you don't mind elaborating, how did you connect DroneKit and Mission Planner using the same connection? I can try to do this tomorrow to see if it fixes the problem on my end as well.

Run through MavProxy. Connect mavproxy with one input and two outputs, something like:
mavproxy.py --master={path to your radio, like /dev/ttyUSB0} --out=udpbcast:{ip of your mission planner pc}:14550  --out=udp:<IP_ADDRESS>:14551
Then, in DroneKit, connect to <IP_ADDRESS>:14551 and in mission planner connect to 14550. Depending on your network and machine setup, you may have to experiment with your udp ins and udp outs. See the mavproxy docs for me.

Interestingly, I was not able to get a UDP broadcast to work. I have been using Mission Planner and my script using TCP ports without any issues, however.
Each of my tests included the following setup:

Run MAVProxy, connect to the UAV
Connect Mission Planner to the MAVProxy instance running
Connect my script to the MAVProxy instance running

When I was ran my script in Python 3 (version 3.6.2), setting wait_ready=False in connect() caused none of the parameters to load. Although it fixed the timeout error, it broke the program since none of the parameters would load. Similarly frustrating, I could not get the parameters to load using wait_ready=True. Trying to run wait_ready=True caused a timeout after 30 seconds. Once I switched my program to Python 2, however, setting wait_ready=False successfully loaded the parameters. I was able to connect to my UAV, and use Mission Planner at the same time as my script. I will try setting wait_ready=True in Python 2 tomorrow, and I will add another comment with my testing results.
@custeve Are you running Python 2 or Python 3?

Using Python 2 fixed this issue. Once I switched back to Python 2 and added the wait_ready=True to the waypoint download command as well as the connect command, nothing timed out. It appears there is a bug somewhere in the Python 3 implementation that causes DroneKit to time out.

I still get this error with my UAV if I use Python 3.
Will this be fixed soon? It is forcing me to continue to use Python 2.

I only test it on Python 2 and got the error, DK's support for Python 3 is still beta (though it should be supported and Python 2 should be dropped).

@DarkmatterVale DK isn't getting any great resource. However if "someone" puts in a PR to fix this we'll certainly review it.

Assuming that your connection string is ok, sometimes you just need to extend the timeout period to get rid of the issue. Here is what I do:

vehicle = connect(..., wait_ready=None) --> or do not set wait_ready
vehicle.wait_ready(True, timeout=<timeout_in_seconds>)

This will connect with the vehicle and then wait until all the main messages have been received up to timeout_in_seconds

when I run :
2. vehicle.wait_ready(True, timeout=<timeout_in_seconds>)
it fails like: File "mission_basic.py", line 39
vehicle.wait_ready(True,timeout=<timeout_in_seconds>)
^
SyntaxError: invalid syntax

I set timeout=180, and then still
Traceback (most recent call last):
File "mission_basic.py", line 38, in
vehicle.wait_ready(True, timeout=180)
File "/home/clickmox/.local/lib/python2.7/site-packages/dronekit/init.py", line 2375, in wait_ready
timeout)
dronekit.TimeoutError: wait_ready experienced a timeout after 180 seconds.
is that because possibly my pymavlink is the version of 2.4.1?

Having same problem in Python 2 with continuous wait_ready calls when waiting home_location to arrive to DK. I have serial connection through pixhawk cube. All modules up to date.

anyone solved this? im having the same issue

I tried to re-request the next waypoint when timeout occured and worked fine. I will create a PR but I don't think it will be merged.

@eminakgun could you please elaborate on what you meant by "re-request the next waypoint"?
I experienced this same issue yesterday and am looking into workarounds as well

#1039 is the implementation here.


Question: Based on QTY?

Thank you for this BRILLIANT idea. It has often been asked of me about doing this same filter but based on QTY > 0 rather than magento's in_stock (because we accept backorders)

Closed due to inactivity from the OP.

Sorry I didnt see your initial reply, let me review again and get back to you, sorry for that

I remember now, some sites I work with request this feature but allow backorders, so products do not trip the flag. So its for use cases where "we do not have it because we have 0" so we want to ship what we have in stock


Add Regularization Modules for LSTM

Adding Regularization modules for LSTM.  Thank you!

[x]  Weight-dropping
[x]  Embedding Dropout
[x] Temporal Activation Regularization
[x] Temporal Averaging

CC: @daemon

@daemon , Please check now. Thanks!

LGTM.


Enable ruby test for already released features

Description
Enable ruby tests for already released features.
This change goes in coordination with the addition of new endpoint to the ruby tests apps https://github.com/DataDog/system-tests-apps-ruby/pull/6
A part from enabling new tests I also added some test metadata on certain standard tags tests.
Check list
Your PR is not ready to be reviewed? Please save it as a draft :pray:

[x] Follows the style guidelines of this project (See how to easily lint the code)
[ ] CI is passing
[ ] There is at least one approval from code owners

Yes to all? Feel free to merge it whenever you want :heart:

Hi, waiting for the CI to be ok, I change this PR to a drfat (we are spammed by PR ready to be reviewed).
If you struggle to understand an issue, do not hesitate to ping me!


fix: allow Uint16|8Array for binary data

Previously we only considered Uint32Array binary data. This was an oversight. This fixes that issue.
Fixes: https://github.com/cloudevents/sdk-javascript/issues/491
Signed-off-by: Lance Ball<EMAIL_ADDRESS>

Updated with your suggestions @grant and CI is passing. PTAL


Implement wrapping functions as iterators

This is the first mergeable PR using the ideas from #244.
Motivation:

no_std. The current implementation of wrap_first_fit does not require allocation at any stage, and so can be used in no_std contexts if we support that in the future.
Avoiding allocations for the return value, for efficency/performance reasons.
Wrapping without creating a slice. wrap_first_fit and wrap_optimal_fit now do not require the fragments to be stored contiguously, and instead simply require any iterator over fragments. This is done without losing any performance (in theory at least). This allows piping the result of splitting functions directly into wrapping functions without collecting into a vector first.
Owned data. It is more flexible to return owned fragments from these functions rather than subslices, as with slices it forces the data to be immutable.

Implementation notes:

I had to implement Fragment for &F so that passing a slice into wrap_first_fit won't break (as iterating over a slice produces references).
The WrapFirstFit and WrapOptimalFit iterators iterate over (F, bool) tuples where the bool indicates whether it's the last fragment of its line; from experience of porting this library's code and tests to the new system it is about equally easy to use.

Oh no, performance has regressed for wrap_first_fit by up to 50% :(. Optimal fit performance hasn't changed at least (in a couple benchmarks it even improved slightly).
There are three places where we allocate vectors that could be replaced with iterators after this change: in break_words, after we call wrap_first_fit/wrap_optimal_fit in wrap, and when generating the list of lines in wrap. Hopefully doing that will improve performance - but I'll leave it for another PR.

Hey @Koxiaet, thanks for putting this up. It'll take a bit to digest it with Christmas coming up :smile:
i'll try and test it and drop some comments over the next few days.

Oh no, performance has regressed for wrap_first_fit by up to 50% :(. Optimal fit performance hasn't changed at least (in a couple benchmarks it even improved slightly).

When i was playing with this in the past, I remember that the trade-off between using vectors and iterators wasn't always clear. Sometimes allocating a few vectors and iterating over them is faster than doing everything with iterators. I'm not 100% sure how it works, but I guess it's a matter of having more complex code vs using more memory upfront.

There are three places where we allocate vectors that could be replaced with iterators after this change: in break_words, after we call wrap_first_fit/wrap_optimal_fit in wrap, and when generating the list of lines in wrap. Hopefully doing that will improve performance - but I'll leave it for another PR.

It would be interesting to see the impact of this on the performance. But please separate this into other commits so that you can easily move it to a separate PR.

no_std. The current implementation of wrap_first_fit does not require allocation at any stage, and so can be used in no_std contexts if we support that in the future.

Do you have such a use case? While it would be technically cool to support no_std, nobody has asked for such a feature yet. I really like it from the aspect of better performance and less memory consumption, but if it makes things much more complex, then I prefer allocating a few Vecs once in a while :-)

No, I do not have a use case, and I agree we shouldn't bother actually supporting it until someone asks. But designing it this way would avoid breaking changes should someone ask. Also I still like iterators over Vecs; this is core after all - not the primary way to consume the crate - and an extra iterator type isn't that complex.

It's worth mentioning that the previous implementation was technically incorrect; if the fragments' widths' had been nearing the integer limit, it would have had all sorts of problems with integer overflow. On the other hand, this version only uses checked arithmetic which does degrade performance but allows it to be more correct. I benchmarked this version using the (incorrect) unchecked arithmetic and performance increased by up to 10%.

It's worth mentioning that the previous implementation was technically incorrect; if the fragments' widths' had been nearing the integer limit, it would have had all sorts of problems with integer overflow.

Yeah, I'm fighting with those overflow problems in #247...
My thinking is that wrap_optimal_fit should handle fragments with a combined width fitting in an usize. This fails today and you're right that an iterator version of wrap_first_fit would be able to handle such "over-sized" strings.
Allowing the input string to come from an iterator has been discussed in #224 — not for the purpose of wrapping strings larger than usize, but for the purpose of consuming the wrapping input which is yielded bit by bit.

No, I do not have a use case, and I agree we shouldn't bother actually supporting it until someone asks. But designing it this way would avoid breaking changes should someone ask.

Yes, good point. I think we agree that a fully iterator-based design is the most flexible and powerful design. My "plan" was roughly to use iterators for all the helper functions and then let wrap_optimal_fit switch to a Vec while wrap_first_fit could continue to use the iterator.
It's of course always a question if the flexible solution is reasonable in terms of complexity and performance. The code already got slower when I introduced the Fragments in #221: the old messy code that did everything in one big loop was about twice as fast as the current code.

Also I still like iterators over Vecs; this is core after all - not the primary way to consume the crate - and an extra iterator type isn't that complex.

Yeah, fair enough.

I looked at the flamegraph to try and find what was causing the slow down. Here is the flamegraph on the old code:

And here is the flamegraph on the new code:

break_words still takes up most of the time spent wrapping, but now lots of time is spent collecting WrapFirstFit into a vector. Of that time, about half of it isn't labelled, so I can only assume that it's reallocating - which somewhat makes sense, the new vector is much much longer than the old one since it stores an entry for each fragment rather than an entry for each line. And a quarter is running ptr::write. Luckily, these won't exist once we remove the collects so I don't have to worry much about it.
But the time spent on next is still a bit more than the time that was spent on wrap_first_fit. This is mostly due to Peekable - maybe removing that would make it go faster? And the time spent in WrapFirstFitInner::next itself is tiny, so it's probably not worth trying to optimize that.

@mgeisler Please can you finish reviewing this? It's starting to conflict.

@mgeisler Please can you finish reviewing this? It's starting to conflict.

Good morning! Yes, I've also had this on my mind the last few days. The conflicts should be trivial: I just moved the wrap_optimal_fit function to its own hidden module so that I could make the smawk dependency optional in #261.
You mentioned integer limits above and I've put up #259 to handle this. It's not merged yet since I haven't heard back from @robinkrahl if this is approach is really better than a simple panic!("Stop using long fragments") at the start of wrap_optimal_fit. But fixing crashes has been my first priority and it's something that I would like to get a release out for soon.

Luckily, these won't exist once we remove the collects so I don't have to worry much about it.

I'm not sure I understand this? The benches/linear.rs benchmarks use fill to ensure that the strings from wrap are collected into a big String. So I think you end up paying for collecting in any case, no? If not in wrap_first_fit and wrap_optimal_fit like today, then in whoever calls them.
Today the input and output types of the helper functions look like this (where I wrote Iterator<T> instead of Iterator<Item = T>):

Function
Current input
Output
Your input
Output

find_words
&str
Iterator<Word>
&str
Iterator<Word>

split_words
Iterator<Word>
Iterator<Word>
Iterator<Word>
Iterator<Word>

break_words
Iterator<Word>
Vec<Word>
Iterator<Word>
Vec<Word>

wrap_*_fit
&[Fragment]
Vec<&[Fragment]>
Iterator<Fragment>
Iterator<(Fragment, bool)>

The helpers stay the say, but the wrap_*_fit functions now take and return iterators. So an input &str goes through these transformations inside of textwrap:wrap:
let line:          &str           = ...;
let words:         Iterator<Word> = find_words(text);
let split_words:   Iterator<Word> = split_words(words);
let broken_words:  Vec<Word>      = break_words(split_words);
let wrapped_lines: Vec<&[Word]>   = wrap_first_fit(&broken_words);
let cow_lines:     Vec<Cow<str>>  = ... // loop over each wrapped line and slice `line`

With your changes, the work is shifted a bit since wrap_*_fit return an iterator:
let line:          &str                   = ...;
let words:         Iterator<Word>         = find_words(text);
let split_words:   Iterator<Word>         = split_words(words);
let broken_words:  Vec<Word>              = break_words(split_words);
let wrapped_words: Iterator<(Word, bool)> = wrap_first_fit(&broken_words);
let cow_lines:     Vec<Cow<str>>          = ... // loop over each Word, check if eol is set and slice `line`

The wrap function is now the one that keeps track of when the lines are done — before this was done by wrap_*_fit since they returned slices of words. Put differently, your approach seems to delay the work a little so that more of it is done when the iterator is advanced in wrap instead of doing it when words are wrapped in wrap_*_fit. Do I understand that correctly?
As a whole, I'm concerned about the 50% slowdown for the otherwise fast wrap_first_fit function. My impression is that putting iterators on top of iterators only work well up to a limit. I don't know why the new version is slower — perhaps because you turn the iterator into a vector here:
        // This currently isn't as efficient as it could be as we collect into a vec even when it
        // isn't necessary.
        let wrapped_words = match options.wrap_algorithm {
            core::WrapAlgorithm::OptimalFit => core::wrap_optimal_fit(broken_words, line_lengths)
                .terminate_eol()
                .into_vec(),
            core::WrapAlgorithm::FirstFit => core::wrap_first_fit(broken_words, line_lengths)
                .terminate_eol()
                .collect(),
        };

As you mention, this vector is much bigger than the old per-line vector.
This points to a problem I ran into with iterators: it's super annoying to match up the types between different iterators. Whereas one vector of Words is the same type as any other vector of Words, iterators almost always end up being different types. So we cannot do
        let wrapped_words = match options.wrap_algorithm {
            core::WrapAlgorithm::OptimalFit => core::wrap_optimal_fit(broken_words, line_lengths),
            core::WrapAlgorithm::FirstFit => core::wrap_first_fit(broken_words, line_lengths),
        };

since the two functions will return different iterators — but we can easily assign the results to the same variable when we collect into a vector.
So if wrap should return an iterator too, then it would have to be a cleverly designed type which knows about the two wrapping algorithms.

So I think you end up paying for collecting in any case, no?

You will end up collecting at least once, but at the moment there are three more collects than necessary - break_words, wrap and wrap_first_fit are all collected (or push to a Vec) when they could use iterators. That's what I meant by removing the collects.

The wrap function is now the one that keeps track of when the lines are done — before this was done by wrap_*_fit since they returned slices of words. Put differently, your approach seems to delay the work a little so that more of it is done when the iterator is advanced in wrap instead of doing it when words are wrapped in wrap_*_fit. Do I understand that correctly?

I wouldn't consider my wrap to do more or less work, it just does it differently. While the code before had to iterate over the lines and then further iterate over each fragment in the line, my code iterates over everything in one loop. There is a bit more state to keep track of as a result, but it's only one number. If you look at the number of lines in wrap my implementation is only a couple lines longer than before. And if you look at fill_inplace, my version reduces the number of lines of code significantly.

I don't know why the new version is slower — perhaps because you turn the iterator into a vector here:

This is what I was referring to when I was talking about the collects earlier; I also suspect that.

It's super annoying to match up the types between different iterators.

So if wrap should return an iterator too, then it would have to be a cleverly designed type which knows about the two wrapping algorithms.

Yes, agreed. I have a plan to make an enumeration type over WrapFirstFit and WrapOptimalFit. I also want to add a method on WrapAlgorithm that returns this enumeration type given the broken words and line lengths, which might be useful.

If you look at the number of lines in wrap my implementation is only a couple lines longer than before. And if you look at fill_inplace, my version reduces the number of lines of code significantly.

Yes, and it's faster than before for small strings:
String lengths/fill_inplace/400
                        time:   [2.8580 us 2.8652 us 2.8708 us]
                        change: [-12.352% -11.913% -11.510%] (p = 0.00 < 0.05)
String lengths/fill_inplace/2400
                        time:   [17.235 us 17.247 us 17.258 us]
                        change: [-0.2181% +0.1055% +0.3754%] (p = 0.51 > 0.05)
String lengths/fill_inplace/6400
                        time:   [45.494 us 45.542 us 45.594 us]
                        change: [+0.1729% +0.4338% +0.7480%] (p = 0.00 < 0.05)

I generated the above with
$ git checkout 0.13.1
$ cargo bench --bench linear /6400 -- -s 0.13.1
$ git checkout Koxiaet-wrap-iterators
$ cargo bench --bench linear /6400 -- -b 0.13.1

So that's indeed a nice win :+1:

I wouldn't consider my wrap to do more or less work, it just does it differently. While the code before had to iterate over the lines and then further iterate over each fragment in the line, my code iterates over everything in one loop. There is a bit more state to keep track of as a result, but it's only one number.

I see that it's roughly doing the same since the tests pass :smile: However, it's still somehow ~30-35% slower than before:
String lengths/fill_first_fit/400
                        time:   [7.6960 us 7.7117 us 7.7376 us]
                        change: [+34.473% +34.829% +35.181%] (p = 0.00 < 0.05)
String lengths/fill_first_fit/2400
                        time:   [40.316 us 40.360 us 40.406 us]
                        change: [+32.506% +32.926% +33.325%] (p = 0.00 < 0.05)
String lengths/fill_first_fit/6400
                        time:   [109.63 us 109.71 us 109.81 us]
                        change: [+32.979% +33.342% +33.697%] (p = 0.00 < 0.05)

So this means this PR is offering more code :-1:, more public API surface :-1:, more future flexibility :+1:, potential future savings :+1:, and an immediate performance regression :-1:. The balance of this seems to be potential future gains and we pay for this with more complexity now.
I think the way forward is to simplify this change as much as absolutely possible. Rip out anything that isn't essential — send PRs with the uncontroversial parts such as your nice explanation of the cost matrix. You also added a TODO about making fill_inplace take a &mut str: that seems like a great idea and I don't know why I didn't use that signature already. Push that to separate PRs so that this PR becomes crystal clear and focused.
I cannot expect you to know the history of the library, but as I've mentioned before, the library changed a lot between version 0.12 and 0.13. I hated the way the code had grown to be in version 0.12.1: it was messy, overly complex, and inflexible. On the other hand, I'm so far very happy with the cleanups I've done in the last few months. So this is why I'm pushing back on what seems to be "regressions" compared to these PRs:

#201: Let functions return "impl Iterator" to hide iterator structs.
I removed such structs from the API:

https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L377
https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L415

If the explicit return types can be replaced with -> impl Iterator, then please do so.
If you can replace the structs with
let mut some_iterator_state = "";
let mut more_state = 0;
return std::iter::from_fn(move || {
    // lovely self-contained logic
}

then even better! I found the old code to be a maze and I'm afraid it could move in that direction again.

#221: Reformulate wrapping in terms of words with whitespace and penalties.
I got rid of a nested iterator struct  here:

https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L435-L446

It held a ton of state and though I tried my best to document it, I ended up with page after page of code like this:

https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L478-L512

The current code is much more direct while also being much more flexible. I think there are none of these small boiler-plate functions left:

https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L377-L382
https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L415-L423
https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L440-L446
https://github.com/mgeisler/textwrap/blob/0.12.1/src/lib.rs#L465-L468

There was so much repetition in the old code, duplicated docstrings everywhere, etc. I'm not saying you're duplicating things, but every struct ends up being part of the public API and as such it gets a docstring and its own little page on docs.rs.

So it seems much of the conflict here is over whether to use impl Iterator or manual iterator types. I prefer the latter for four reasons.
First of all, an argument very similar to the ones I used in the comments above - lines of code != cost. Complexity is cost. And although manual iterator types introduce many more lines of code, its complexity is identical to impl Iterator. Having another type in the API surface is a bit more complex than not having it, but it's very minor, and when there is a function wrap_first_fit as well as a struct WrapFirstFit, users will know exactly what the struct does and will know of its importance.
Second, being able to name types is useful. Yes, this limitation won't exist in a few years once impl Trait in type aliases becomes stable, but it's not stable yet and even when it is it will require extra boilerplate for the user, as well as inconsistent naming of the iterator among different users. This is even relevant for us - in order to make a iterator that enumerates WrapFirstFit and WrapOptimalFit, we require being able to name those types.
Third, precedent. Once again I will point to the standard library, which never uses impl Trait in return position. But it is also a very common pattern to have named iterator types for user crates - the Rust API guidelines on naming has a section on this. And just in general named iterators are common, there are many examples in the ecosystem like unicode-segmentation, itertools, etc.
Fourth, it allows us to implement traits conditionally. A trait impl like "only implement FusedIterator when the underlying iterator implements FusedIterator" is simply not expressable with impl Trait. This is also relevant when implementing Clone, which is recommended to implement by the C-COMMON-TRAITS guideline.

So it seems much of the conflict here

I think the main issue is that this change enables potential future flexibility and that the cost for this is paid now. The performance regression for wrap_first_fit is a regression and I don't see enough gain yet to offset this cost.
I understand that this is a stepping stone towards a more streaming API. In the limit, this could allow wrapping text from an impl Iterator and we could return impl Iterator as well without materializing a Vec at any point (as you've pointed out). This is nice, though it conflicts with a few things:

We don't know how expensive this really is.
We probably don't really need this since wrapping wtih a Vec is super fast.
We have a wrapping algorithm which needs a Vec anyway...

This makes me say that we need some real use case to pay the price of the performance hit. Ideally, the change should be a performance improvement — which would justify what I see as downsides in terms of code organization and more types in the public API.
I had a streaming API in the past — and crates like clap would simply wrap a &str and return a String. Furthermore, https://github.com/clap-rs/clap/issues/1365 mentions that textwrap is already showing up as a largeish dependency of clap, and it would be a shame if textwrap ended up being dropped because of too much complexity.

Having another type in the API surface is a bit more complex than not having it, but it's very minor

I tried to point to the old code above to show how it was much more annoying to work with — the artificial split between the function returning the iterator, the struct itself, and the impl Iterator block moves three things apart which ought to be one. It was a step forward for me when I got rid of that split — and I'm reluctant to reintroduce it.

Third, precedent. Once again I will point to the standard library, which never uses impl Trait in return position. But it is also a very common pattern to have named iterator types for user crates - the Rust API guidelines on naming has a section on this. And just in general named iterators are common, there are many examples in the ecosystem like unicode-segmentation, itertools, etc.

Is this not simply because the code was written long before -> impl Iterator was a thing? I would expect all new libraries to not expose the underlying iterator struct.

So it seems much of the conflict here

I think the main issue is that this change enables potential future flexibility and that the cost for this is paid now. The performance regression for wrap_first_fit is a regression and I don't see enough gain yet to offset this cost.
I understand that this is a stepping stone towards a more streaming API. In the limit, this could allow wrapping text from an impl Iterator and we could return impl Iterator as well without materializing a Vec at any point (as you've pointed out). This is nice, though it conflicts with a few things:

We don't know how expensive this really is.
We probably don't really need this since wrapping wtih a Vec is super fast.
We have a wrapping algorithm which needs a Vec anyway...

This makes me say that we need some real use case to pay the price of the performance hit. Ideally, the change should be a performance improvement — which would justify what I see as downsides in terms of code organization and more types in the public API.
I had a streaming API in the past — and crates like clap would simply wrap a &str and return a String. Furthermore, https://github.com/clap-rs/clap/issues/1365 mentions that textwrap is already showing up as a largeish dependency of clap, and it would be a shame if textwrap ended up being dropped because of too much complexity.

Having another type in the API surface is a bit more complex than not having it, but it's very minor

I tried to point to the old code above to show how it was much more annoying to work with — the artificial split between the function returning the iterator, the struct itself, and the impl Iterator block moves three things apart which ought to be one. It was a step forward for me when I got rid of that split — and I'm reluctant to reintroduce it.

Third, precedent. Once again I will point to the standard library, which never uses impl Trait in return position. But it is also a very common pattern to have named iterator types for user crates - the Rust API guidelines on naming has a section on this. And just in general named iterators are common, there are many examples in the ecosystem like unicode-segmentation, itertools, etc.

Is this not simply because the code was written long before -> impl Iterator was a thing? I would expect all new libraries to not expose the underlying iterator struct.

too much complexity.

I think that's the problem here - we have different definitions of "simple" and "complex". I want this API, because I consider iterators to be simpler than vectors, even if they have more lines of code.
I believe that APIs, in order to be maximally simple, should do one thing and one thing only. An API that returns a sequence of values shouldn't do other things like allocate, it shouldn't prescribe a collection type for the user, it shouldn't require that slices of fragments are what the user needs to pass in - these are all out of scope and over-complicate the API. Iterators on the other hand, while they do need more code to implement, are theoretically simpler because they don't do anything other than their singular purpose. So if there's a slight performance loss or there are more types in the public API, so be it. To me it's far better to have a simple API than a fast one or a small one (unless it's really slow of course :smile:).
This situation is an interesting clash between the two philosophies of "do one thing and do it well" and using as few lines as possible :laughing:. I can understand why you'd subscribe to the latter, I just find it a bit reductionist.

textwrap is already showing up as a largeish dependency of clap

Hmm, this is somewhat of a problem. Maybe more aggressive feature-flagging is the way to go? I see you're already doing that a bit with the other PRs in this repo.

moves three things apart which ought to be one

Is having them next to each other in the source not enough? I agree it's not quite as together as if it was all in one function, but it's not that much harder to navigate.

I would expect all new libraries to not expose the underlying iterator struct.

No, the standard library still prefers named types. For example, the recently added core::future::ready and core::future::pending APIs return core::future::Ready and core::future::Pending, even when they could be async fns which would the number of LOC. And those are futures, which even I would usually not support writing named types for.

too much complexity.

I think that's the problem here - we have different definitions of "simple" and "complex". I want this API, because I consider iterators to be simpler than vectors, even if they have more lines of code.
I believe that APIs, in order to be maximally simple, should do one thing and one thing only. An API that returns a sequence of values shouldn't do other things like allocate, it shouldn't prescribe a collection type for the user, it shouldn't require that slices of fragments are what the user needs to pass in - these are all out of scope and over-complicate the API. Iterators on the other hand, while they do need more code to implement, are theoretically simpler because they don't do anything other than their singular purpose. So if there's a slight performance loss or there are more types in the public API, so be it. To me it's far better to have a simple API than a fast one or a small one (unless it's really slow of course :smile:).
This situation is an interesting clash between the two philosophies of "do one thing and do it well" and using as few lines as possible :laughing:. I can understand why you'd subscribe to the latter, I just find it a bit reductionist.

textwrap is already showing up as a largeish dependency of clap

Hmm, this is somewhat of a problem. Maybe more aggressive feature-flagging is the way to go? I see you're already doing that a bit with the other PRs in this repo.

moves three things apart which ought to be one

Is having them next to each other in the source not enough? I agree it's not quite as together as if it was all in one function, but it's not that much harder to navigate.

I would expect all new libraries to not expose the underlying iterator struct.

No, the standard library still prefers named types. For example, the recently added core::future::ready and core::future::pending APIs return core::future::Ready and core::future::Pending, even when they could be async fns which would the number of LOC. And those are futures, which even I would usually not support writing named types for.

I think that's the problem here - we have different definitions of "simple" and "complex". I want this API, because I consider iterators to be simpler than vectors, even if they have more lines of code.

Yes, I think you hit the nail on the head there :smile:  I agree that an Iterator puts less constraints on the inputs and outputs than a Vec or a slice. It's normally great if APIs only makes the minimum assumptions needed — it makes the life easier for the callers.
However, I still think there are a bunch of trade-offs here:

Returning impl Iterator: Simpler for the caller: can only call next() and that's it. On the other hand, it is less clear for that caller what the cost is of calling next() since it's unclear what it does. The two wrapping algorithms illustrate this to some extent: wrap_first_fit has an even performance characteristic where every next() takes about the same time — wrap_optimal_fit has an uneven performance and most of the work is done on the very first call to next(). In other words, the Iterator API is not the natural API for wrap_optimal_fit.

Returning Vec: Prescribes memory allocation for the caller. Less choice for the caller which is simpler in some sense, but it might be the wrong choice if the caller did not intend to keep all the wrapped lines in memory at once. The wrap_first_fit algorithm can produce streaming output very well, so returning a Vec is not the natural API for this function.

Iterators on the other hand, while they do need more code to implement, are theoretically simpler because they don't do anything other than their singular purpose. So if there's a slight performance loss or there are more types in the public API, so be it. To me it's far better to have a simple API than a fast one or a small one (unless it's really slow of course ).

I get the theoretical purity of this — however, I find it hard to agree that all the <F as IntoIterator>::Item: Fragment bounds make the code simpler. It certainly makes it more flexible, but it comes with a cost of extra complexity in my eyes. You can imagine how concerned I was when I introduced the where Opt: Into<Options<'a, S>> bounds... it was a nightmare to convince myself :smile: In the end I went with it since I really liked the much smaller API this gave me — no funny wrap_with_usize and wrap_with_options shims for example.
Similarly, I'm looking for something concrete which is improved by switching things to using Iterators. Could you implement a top-level wrap_iter function for example. I guess it should be hard-coded to use the wrap_first_fit algorithm since this is the one which works well with Iterators.
It would be cool to see a function which can take input in an incremental fashion — that's something which would would require extra refactoring to support today (though the divide-and-conquer approach in #259 seems like it could be generalized).

This situation is an interesting clash between the two philosophies of "do one thing and do it well" and using as few lines as possible . I can understand why you'd subscribe to the latter, I just find it a bit reductionist.

One could argue that wrapping text is the goal of the library :smile: Until very recently, this was all it could do: wrap text. There was an underlying assumptions that the caller would be able to hold both the input and the output in memory. This makes Iterators mostly uninteresting since they add features that are outside of the core domain of the library.
That being said, #224 makes a case for keeping state while wrapping — I mentioned this above somewhere. So yeah, I would like textwrap to play well with situations where the input isn't know up-front or where the output isn't consumed all at once.

textwrap is already showing up as a largeish dependency of clap

Hmm, this is somewhat of a problem. Maybe more aggressive feature-flagging is the way to go? I see you're already doing that a bit with the other PRs in this repo.

Yeah, I started that recently as a reaction to that discussion. I think it's already pretty modular now... While I don't like the idea of including a huge library for a tiny function, I also don't like the idea that people turn off useful functionality in order to make binaries faster to build — that's what we have caches for. I'm used to Bazel at work which makes it feasible to have both a lot of dependencies plus reasonable fast builds due to extensive caching.
I hope that textwrap can still carry its own weight by providing more correct and better wrapping that what people can hand-roll in an afternoon. The corner case bugs I've fixed over the years tells me that wrapping text isn't completely trivial. The second commit already fixed a small bug: https://github.com/mgeisler/textwrap/commit/0e07230c57ca53c92bf2caa87568afddfbe245ce :smile:
In any case, we should keep an eye on this aspect too, even if it's a bit annoying.

moves three things apart which ought to be one

Is having them next to each other in the source not enough? I agree it's not quite as together as if it was all in one function, but it's not that much harder to navigate.

Yeah, it's of course not that much harder, but it just feels like a small step backwards to me.  I think it's a matter of ensuring that things are self-contained. If you see return std::iter::from:fn(...) then you know that the Iterator being returned cannot depend on more than you see right there. It took me a long time to "untangle" the old code and so I'm reluctant to give up on the new structure.

I would expect all new libraries to not expose the underlying iterator struct.

No, the standard library still prefers named types. For example, the recently added core::future::ready and core::future::pending APIs return core::future::Ready and core::future::Pending, even when they could be async fns which would the number of LOC. And those are futures, which even I would usually not support writing named types for.

Okay, thanks, I was not aware of this at all :+1:

I think that's the problem here - we have different definitions of "simple" and "complex". I want this API, because I consider iterators to be simpler than vectors, even if they have more lines of code.

Yes, I think you hit the nail on the head there :smile:  I agree that an Iterator puts less constraints on the inputs and outputs than a Vec or a slice. It's normally great if APIs only makes the minimum assumptions needed — it makes the life easier for the callers.
However, I still think there are a bunch of trade-offs here:

Returning impl Iterator: Simpler for the caller: can only call next() and that's it. On the other hand, it is less clear for that caller what the cost is of calling next() since it's unclear what it does. The two wrapping algorithms illustrate this to some extent: wrap_first_fit has an even performance characteristic where every next() takes about the same time — wrap_optimal_fit has an uneven performance and most of the work is done on the very first call to next(). In other words, the Iterator API is not the natural API for wrap_optimal_fit.

Returning Vec: Prescribes memory allocation for the caller. Less choice for the caller which is simpler in some sense, but it might be the wrong choice if the caller did not intend to keep all the wrapped lines in memory at once. The wrap_first_fit algorithm can produce streaming output very well, so returning a Vec is not the natural API for this function.

Iterators on the other hand, while they do need more code to implement, are theoretically simpler because they don't do anything other than their singular purpose. So if there's a slight performance loss or there are more types in the public API, so be it. To me it's far better to have a simple API than a fast one or a small one (unless it's really slow of course ).

I get the theoretical purity of this — however, I find it hard to agree that all the <F as IntoIterator>::Item: Fragment bounds make the code simpler. It certainly makes it more flexible, but it comes with a cost of extra complexity in my eyes. You can imagine how concerned I was when I introduced the where Opt: Into<Options<'a, S>> bounds... it was a nightmare to convince myself :smile: In the end I went with it since I really liked the much smaller API this gave me — no funny wrap_with_usize and wrap_with_options shims for example.
Similarly, I'm looking for something concrete which is improved by switching things to using Iterators. Could you implement a top-level wrap_iter function for example. I guess it should be hard-coded to use the wrap_first_fit algorithm since this is the one which works well with Iterators.
It would be cool to see a function which can take input in an incremental fashion — that's something which would would require extra refactoring to support today (though the divide-and-conquer approach in #259 seems like it could be generalized).

This situation is an interesting clash between the two philosophies of "do one thing and do it well" and using as few lines as possible . I can understand why you'd subscribe to the latter, I just find it a bit reductionist.

One could argue that wrapping text is the goal of the library :smile: Until very recently, this was all it could do: wrap text. There was an underlying assumptions that the caller would be able to hold both the input and the output in memory. This makes Iterators mostly uninteresting since they add features that are outside of the core domain of the library.
That being said, #224 makes a case for keeping state while wrapping — I mentioned this above somewhere. So yeah, I would like textwrap to play well with situations where the input isn't know up-front or where the output isn't consumed all at once.

textwrap is already showing up as a largeish dependency of clap

Hmm, this is somewhat of a problem. Maybe more aggressive feature-flagging is the way to go? I see you're already doing that a bit with the other PRs in this repo.

Yeah, I started that recently as a reaction to that discussion. I think it's already pretty modular now... While I don't like the idea of including a huge library for a tiny function, I also don't like the idea that people turn off useful functionality in order to make binaries faster to build — that's what we have caches for. I'm used to Bazel at work which makes it feasible to have both a lot of dependencies plus reasonable fast builds due to extensive caching.
I hope that textwrap can still carry its own weight by providing more correct and better wrapping that what people can hand-roll in an afternoon. The corner case bugs I've fixed over the years tells me that wrapping text isn't completely trivial. The second commit already fixed a small bug: https://github.com/mgeisler/textwrap/commit/0e07230c57ca53c92bf2caa87568afddfbe245ce :smile:
In any case, we should keep an eye on this aspect too, even if it's a bit annoying.

moves three things apart which ought to be one

Is having them next to each other in the source not enough? I agree it's not quite as together as if it was all in one function, but it's not that much harder to navigate.

Yeah, it's of course not that much harder, but it just feels like a small step backwards to me.  I think it's a matter of ensuring that things are self-contained. If you see return std::iter::from:fn(...) then you know that the Iterator being returned cannot depend on more than you see right there. It took me a long time to "untangle" the old code and so I'm reluctant to give up on the new structure.

I would expect all new libraries to not expose the underlying iterator struct.

No, the standard library still prefers named types. For example, the recently added core::future::ready and core::future::pending APIs return core::future::Ready and core::future::Pending, even when they could be async fns which would the number of LOC. And those are futures, which even I would usually not support writing named types for.

Okay, thanks, I was not aware of this at all :+1:

To sum up my concerns:

The performance drop.
I would like to see the new code do something which cannot be done today.

To sum up my concerns:

The performance drop.
I would like to see the new code do something which cannot be done today.

the Iterator API is not the natural API for wrap_optimal_fit.

The natural API for wrap_optimal_fit is a reversed iterator. But of course that doesn't exist, so we have to put it into a collection and then reverse it from there. Once we have collected into a vector, we could .reverse() it and return that, but we could also use a sequence of .pop() calls, and both are equally natural. So my solution was the iterator type, that can both serve as an iterator that calls .pop() or can be converted into a Vec after calling .reverse(). This allows users to use both natural options, so I would consider an iterator the natural API for wrap_optimal_fit.

Could you implement a top-level wrap_iter function for example

I could definitely do this - in fact, this can even be done with the current code. You'd just have to replace the for loop with a flat_map. Whether the wrapping functions are iterators doesn't really affect this at all.

It took me a long time to "untangle" the old code and so I'm reluctant to give up on the new structure.

So I think the solution here is to break up the modules - while it's confusing to have it return a named iterator type that's lower down in the next lines that could be for anything else, it makes it much less confusing if it's all in wrap_first_fit.rs. I want to incorporate these ideas in another PR, I have plans for a module structure I think is cleaner than the current one.

I would like to see the new code do something which cannot be done today.

I should mention the original motivation for this PR. I'm writing some code for a UI library whose API looks roughly like this:
struct Text { ... }
impl Text {
    fn new(s: String) -> Self { ... }
}
impl Component for Text {
    fn draw(&self, canvas: Canvas) { ... }
}

The new function will find all the soft line breaks in the text and store their indices (indices because storing many Strings is very inefficient). Then in draw I need to draw all these to the canvas, which can be of any size. With the current API I can't implement Fragment for each pair of indices because they don't have access to the string. However, with this new API I can map the list of indices, which means they will have access to the string and so I can use a type that implements Fragment.

the Iterator API is not the natural API for wrap_optimal_fit.

The natural API for wrap_optimal_fit is a reversed iterator. But of course that doesn't exist, so we have to put it into a collection and then reverse it from there. Once we have collected into a vector, we could .reverse() it and return that, but we could also use a sequence of .pop() calls, and both are equally natural. So my solution was the iterator type, that can both serve as an iterator that calls .pop() or can be converted into a Vec after calling .reverse(). This allows users to use both natural options, so I would consider an iterator the natural API for wrap_optimal_fit.

Could you implement a top-level wrap_iter function for example

I could definitely do this - in fact, this can even be done with the current code. You'd just have to replace the for loop with a flat_map. Whether the wrapping functions are iterators doesn't really affect this at all.

It took me a long time to "untangle" the old code and so I'm reluctant to give up on the new structure.

So I think the solution here is to break up the modules - while it's confusing to have it return a named iterator type that's lower down in the next lines that could be for anything else, it makes it much less confusing if it's all in wrap_first_fit.rs. I want to incorporate these ideas in another PR, I have plans for a module structure I think is cleaner than the current one.

I would like to see the new code do something which cannot be done today.

I should mention the original motivation for this PR. I'm writing some code for a UI library whose API looks roughly like this:
struct Text { ... }
impl Text {
    fn new(s: String) -> Self { ... }
}
impl Component for Text {
    fn draw(&self, canvas: Canvas) { ... }
}

The new function will find all the soft line breaks in the text and store their indices (indices because storing many Strings is very inefficient). Then in draw I need to draw all these to the canvas, which can be of any size. With the current API I can't implement Fragment for each pair of indices because they don't have access to the string. However, with this new API I can map the list of indices, which means they will have access to the string and so I can use a type that implements Fragment.


fix: changed the button text to new when no apps are present in workspace

Description
Reverted the button text from New Application to New when there are no apps present in the workspace
PR fixes following issue(s)
Fixes #27390
Media

A video or a GIF is preferred. when using Loom, don’t embed because it looks like it’s a GIF. instead, just link to the video

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Chore (housekeeping or task changes that don't impact user perception)
This change requires a documentation update

Testing

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration.
Delete anything that is not relevant

[ ] Manual
[ ] JUnit
[ ] Jest
[ ] Cypress

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:
Dev activity

[ ] My code follows the style guidelines of this project
[ ] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[ ] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] New and existing unit tests pass locally with my changes
[ ] PR is being merged under a feature flag

QA activity:

[ ] Speedbreak features have been covered
[ ] Test plan covers all impacted features and areas of interest
[ ] Test plan has been peer reviewed by project stakeholders and other QA members
[ ] Manually tested functionality on DP
[ ] We had an implementation alignment call with stakeholders post QA Round 2
[ ] Cypress test cases have been added and approved by SDET/manual QA
[ ] Added Test Plan Approved label after Cypress tests were reviewed
[ ] Added Test Plan Approved label after JUnit tests were reviewed

/ok-to-test


Question about "Official Advantage/Disadvantage rules"

The wiki doesn't include (correct me if i am wrong) the details of this part so i cannot confirm if it is intended. At least one thing I noticed different from official rules is about combat in the dark.
According to PHB rules (I also checked it in other explanations such as darkvision), the basics here is if a creature cannot see his attacker or target, the attack roll to and from him has disadvantage, and this includes the mechanics of light condition.
In a dark scene, if both the attacker and target don't have the ability to see in the dark, then the advantage and disadvantage cancel and they attack each other normally. If only one with it and the other not, then they are granted with advantage and disadvantage in attack rolls, respectively. This also applies to magic darkness and other sight obscuring effects.
In the vanilla game the case is the lack of ability to see a creature won't grant advantage to attackers to him (apart from stealth and invisibility), so in the first scenario both attack with disadvantage, and in the second, the one  who can see through darkness attack normally. And with the mod "Official Advantage/Disadvantage rules" on ,this doesn't change.
So this might be a bug or an unimplemented feature, hence the issue here above.

Game calculates advantage / disadvantage as sum[ADV] - sum[DIS]. If positive ADV, negative DIS, zero NEUTRAL.
All this setting does is force neutral if there is at least 1 ADV and 1 DIS as official rules.
Darkness and anything related to light is another issue as TA doesn’t follow the rules. Nothing we can do about it.


New unified video IO

Your recent changes are great and have significantly reduced code redundancy. The code is also easier to read now. Here I have another idea that might make it a little better.
The process of extracting video frames is now defined in base_flow_extractor.py and base_framewise_extractor.py, which use ffmpeg to re-encode the video and opencv to read it. But users may need a new feature of extracting a fixed number of frames for a video, which is harder to implement with the current framework. And, users may want to be able to extract features faster, so could you consider using a more efficient API to read videos (e.g., decord). Moreover, specifying fps to extract features requires re-encoding the video and generating temporary files. Is this necessary? Can we meet the fps by extracting frames at intervals rather than re-encoding it?
The ideal API may looks like:
# when fps = 2
frame_extractor = FrameExtractor("sample.mp4", batchsize=16, fps=2)
# when we want to extract 12 frames
frame_extractor = FrameExtractor("sample.mp4", batchsize=16, total=12)
# when extracting flow
frame_extractor = FrameExtractor("sample.mp4", batchsize=16, overlap=1)

for batch, time_stamp in frame_extractor:
    batch = transform(batch)  # or as a parameter for the frame extractor
    _run_on_a_batch(batch)

In summary, this new feature can further reduce code redundancy, improve speed and provide new extracting methods.
I would love to make this improvement, but I've been busy lately and probably won't be able to do it until September.

Hi there!
Yees! I have been thinking about IO a lot and wanted to change it since the very inception of this project. Also, note that some of the features rely on cv2 while others on torchvision.something.read_video.
We need to unify it, and, hopefully, make it faster by picking another IO tool.
Also, we need to merge the frame-wise base extractor and flow base extractor after the io algorithm is unified.

specifying fps to extract features requires re-encoding the video and generating temporary files. Is this necessary?

I thought so and it is quite plain and easy to understand. I do admit that we could do a better job there. However, first, I need to figure out how FFmpeg converts 30 fps to 25, for instance, or some other non-trivial resampling, and, second, we need to reimplement the "skipping/duplicating" algorithm here. It seems like a rather overkill at this stage but I think you have a more clear view of how to do it. Could you share it? I would love to read it.

On a side note,
I dislike having cv2 as a dependency and the fact that it is quite slow but I like that I could read a video frame by frame and extract features from a batch. Note, if you will try to load the whole video with a reader and try to process it, we will quickly run out RAM.
Recently, I’ve been trying many video IO scripts:

ffmpeg-python and reading the frame from mp4 directly by converting the binary into numpy. As implemented in Antonie Miech’s S3D. Main con: need to load the whole video in RAM.
Extracting frames with ffmpeg cmd and load by an image IO. Main con: slow but also gross.
torchvision.read_video is good enough +/- it reads audio (convenient but sometimes unnecessary - makes extraction slower). Main con: reads the whole video and does not work out of the box after torchvision installation (has pyav as a dependency)
the brand new: torchvision.io.VideoReader. It is incredibly fast and I love it. It allows you to iterate either audio or RGB tracks of a mp4 frame-by-frame. However, if we could skip some frames in RGB, audio is read by 2048 or 1024 samples which I don't know how to resample on the fly (e.g. 44100 into 48000), the same as 30 fps into 25 fps. Another con is dependency hell. I installed it once and I don't even know how to do it again anymore and it depends on the latest torch(vision). One may find some issues in torch/vision repo regarding this new API. Another slightly strange local issue is related to the reconstruction of the audio read this way (mp4 -> VideoReader('audio') -> wav_tensor -> torchvision.write_video -> .wav file). It produced a higher-pitched audio in my environment yet worked on google colab. In other words, it is raw but I like it.
I think I tried Decord but never managed to fill in the needs that I have. I don't remember exactly my issue with it but I gave it a try.

I will rename the issue to make it more precise if I understand your suggestion correctly.

Thank you for make the title more clear!
Regarding changing the FPS, I need to find more information about it. My initial idea was to first calculate the number of frames needed for the new fps, then use np.array(np.linspace(0, frame_num-1, new_frame_num)) to get the indices of the extracted frames, and finally extract the needed frames based on the indices. It has error in some cases, i.e. the frames obtained may not correspond exactly to the timestamp. The re-encoding method is more accurate, and the above method is faster. I'm not sure if this error can be ignored or how everyone else handles it.

torchvision.io.VideoReader looks good! But it is really sad that we must use a high version of torch.
I think it's very good if Decord meets the needs.
We could try to find the best IO script as a start.

By the way, ChangingFrameRate may be helpful.

I did a benchmark on Decord, mmcv and OpenCV.
It seems OpenCV is faster when reading short videos sequentially, and decord is faster when reading long videos sequentially. These three modules don't load full video. Here are some questions:

The decord might be faster when using gpu. I will try this.
Never successfully install torchvision.io.VideoReader 😞

Oh, thanks a lot!
A few things I would like to clarify for myself:

When we rely on video readers such as those that you benchmarked on, do we need to check memory footprint on the init call (such as cap = cv2.VideoCapture("long.mp4"))? These don't load the video into memory, do they? These are generators, not iterators - am I right?
I see quite a small difference in speed performance (at most ~20% for random access). I think if it is the case, we could pick the one that has less dependency pain. Which one do you think is better from this perspective?
Yes, I think before the last torch update you could run VideoReader on colab. I think it is no longer possible even there. This is very disappointing but I think the torch team knows about it.

For the first point, yes, I think we need to check if the reader loads the whole video into RAM, and they do not. I don't really understand the difference between the generator and iterator you mentioned, but during the file processing, these readers probably only read some basic information about the video when they are initialized, after which they move the pointer according to the user input (I'm not sure of the details either).
For the second and the third points, I think we'd better keep OpenCV until torchvision.io.VideoReader is more easily accessible.
I also tested the gpu version of decord afterwards, and while it is relatively easier to install (compared to torchvision) and it has a very high boost when reading long videos sequentially, it seems to read the entire video into memory.
We may just need to change the existing IO API, but I still have a question, should we re-encode the video and use all frames after re-encoding? Or should we just read the video and extract the frames we need. Perhaps you would have more experience in this area? Do you know how other researchers have handled this?

I don't really understand the difference between the generator and iterator you mentioned.

Iterators keep information in RAM and you just iterate through values in some way, e.g. [1, 2, 3] is an iterator. Generator generates new value. It means it does not need to store the whole list/video in RAM. I think the famous example of this differentiation is the range() function in Python 2 (list) and python 3 (generator).

should we re-encode the video and use all frames after re-encoding?
Yes, unless we will figure out a clean and well-known way of dealing with non-trivial source/target fps. For instance, trivial: from 30 to 10 (going in triplets: ABC and dropping A and C); non-trivial: from 24 to 15. Here, I would rather rely on existing implementations like ffmpeg to do it for me rather than write something with many edge cases myself as bugs can occur.

Usually, researchers just pre-encode the whole folder to the same fps. For install, scrap YouTube, then re-encode with ffmpeg to 25 fps. I think, the reencode_wtih_another_fps does this but it just does it every time a person processes a video. It is negligible if the user extracts the same features once but can be problematic if they need 10 different features as this will be run 10 times. Well, I think it is ok still, the process is very fast and not the bottleneck, so I don't share your concerns, really.  Maybe we could replace it with something more pythonic like ffmpeg-python. It is the same but you don't need to write the command in a ffmpeg CLI kind of way. Take a look at ffmpeg-python, do you think it would be nice to replace the current script with it (I could do it, don't worry; the docs for this library are quite horrible by the way)?

Thanks for your reply, and I agree with you that using ffmpeg solves the fps problem. I wasn't worried about the brevity of the code. In fact using command line calls to ffmpeg works quite well.

it seems to read the entire video into memory.

Ok, I decided to double-check it because I think it wasn't my impression the last time I checked.
Thanks for providing the colab MWE, I could quickly start experimenting.
Here is my 2nd honest attempt to use decord (it has some comments):
https://colab.research.google.com/drive/1sF4BjvFMkU6uBtupKgZ8_2ROCZBuEd-h?usp=sharing
By the way, did you compile it for GPU use or installed it in some other way?

Sorry for the late reply, but I'm very busy these days as I'm at the beginning of the semester.
Your analysis of the decord is much better than mine, and I also feel that the CPU version decord is not much better than the cv2 for now.  I compiled the source code of decord for GPU use. You can use the following code to install the GPU version of decord on colab:
!sudo apt-get update
!sudo apt-get install -y build-essential python3-dev python3-setuptools make cmake
!sudo apt-get install -y libavcodec-dev libavfilter-dev libavformat-dev libavutil-dev
!git clone --recursive https://github.com/dmlc/decord
%cd decord
!mkdir build
%cd build
!cmake .. -DUSE_CUDA=ON -DCMAKE_BUILD_TYPE=Release
!make
%cd /content/decord/python
!python setup.py install --user
%cd /content

I tried the GPU version decord with your colab notebook. The results are as follows:
Filename: tmp.py

Line #    Mem usage    Increment  Occurrences   Line Contents
=============================================================
     5   75.531 MiB   75.531 MiB           1   @profile
     6                                         def my_func():
     7  204.941 MiB  129.410 MiB           1       vr = VideoReader('long.mp4', ctx=gpu(0))
     8  270.617 MiB   65.676 MiB           1       batch = vr.next()
     9  270.617 MiB    0.000 MiB           1       batch = vr.next()
    10  270.617 MiB    0.000 MiB           1       batch = vr.next()
    11  270.617 MiB    0.000 MiB           1       batch = vr.next()
    12  270.617 MiB    0.000 MiB           1       batch = vr.next()
    13  270.617 MiB    0.000 MiB           1       batch = vr.next()
    14  270.617 MiB    0.000 MiB           1       batch = vr.next()
    15  270.617 MiB    0.000 MiB           1       batch = vr.next()
    16  270.617 MiB    0.000 MiB           1       batch = vr.next()
    17
    18  276.551 MiB    0.000 MiB         669       for i in range(1024):
    19  276.551 MiB    5.934 MiB         669           batch = vr.next()
CPU times: user 40.3 ms, sys: 11.1 ms, total: 51.5 ms
Wall time: 5.13 s

I also tried the CPU version with your code. The results are slightly different from yours in the notebook:
Filename: tmp.py

Line #    Mem usage    Increment  Occurrences   Line Contents
=============================================================
     5   76.547 MiB   76.547 MiB           1   @profile
     6                                         def my_func():
     7  107.945 MiB   31.398 MiB           1       vr = VideoReader('long.mp4')
     8  369.211 MiB  261.266 MiB           1       batch = vr.next()
     9  421.965 MiB   52.754 MiB           1       batch = vr.next()
    10  421.965 MiB    0.000 MiB           1       batch = vr.next()
    11  425.828 MiB    3.863 MiB           1       batch = vr.next()
    12  425.828 MiB    0.000 MiB           1       batch = vr.next()
    13  437.938 MiB   12.109 MiB           1       batch = vr.next()
    14  437.938 MiB    0.000 MiB           1       batch = vr.next()
    15  438.707 MiB    0.770 MiB           1       batch = vr.next()
    16  438.707 MiB    0.000 MiB           1       batch = vr.next()
    17
    18  455.441 MiB    0.000 MiB         669       for i in range(1024):
    19  455.441 MiB   16.734 MiB         669           batch = vr.next()
CPU times: user 220 ms, sys: 40.2 ms, total: 261 ms
Wall time: 37.2 s

The GPU version uses less memory and is significantly faster (over 7 times faster).
Here are the changed codes: https://colab.research.google.com/drive/1rs80lFKHL9_kASY4DqwqnmxfxUq26zCg?usp=sharing

#70


Consider using prefix code and zigzag encoding for variable-length integers

Proposal: can we broaden this issue to discuss what variable-length integer encoding in general would be most appropriate?
I was unfamiliar with zigzag encoding and what advantages it would have over the current method, so I searched for "leb128 vs zigzag". I found these two related discussions about LEB128 being used in WASM, both of which raised a number of points that seemed relevant to the topic:
https://news.ycombinator.com/item?id=11263378
https://github.com/WebAssembly/design/issues/601
A quick summary of what stood out to me:

zigzag encoding/decoding is allegedly faster for signed integers than plain signed LEB128
implementations for prefix-based varints are generally 2x-3x faster than LEB128
LEB128 allows for locating the start and end with random access, prefix-based varints don't
In the context of UTF8, having non-canonical representations has lead to security issues.

Technically LEB128 also lacks canonical encodings for numbers: zero for example would typically be encoded as 00, but technically 80 00, 80 80 00, ... 80 80 ... 80 00 also work. Should this be forbidden? (in practice this means disallowing LEB128 to have trailing 80s followed by 00 for positive values, and similarly FFs + 7F for negative values. Can't think of the top of my head if this applies to zigzag encoding)

I have no strong opinions on any of this, and I do not know what the most relevant criteria are to decide on a varint encoding for muon anyway. But these points they seemed relevant enough to bring up (even if muon sticks to plain LEB128 you can at least say that you looked at alternatives, right?)
PS: for anyone else unfamiliar with zigzag encoding, this blog and this SO question helped me out

I think i'll give it another try


Possibly incorrect value of the name property inside pyproject.toml file

After cloning the repository and running poetry install the following is printed:

Perhaps, the property name should be changed as follows:
# pyproject.toml:

-name = "gitbug-java"
+name = "gitbug"

After the property is changed the command poetry install executes with no warning and all the following commands from the instructions are executed normally.

Hi @Vladislav0Art,
Thanks a lot for your report and interest :)
#17 should fix this, the warning is there because we forgot to disable the package mode for this project.
Nonetheless, you can still use the version under main as the warning does not have any further implications for the usage of GitBug-Java.


chore(git): add ISSUE_TEMPLATES

Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 82.11%.  Consider uploading reports for the commit 422f68d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #45   +/-   ##
=======================================
  Coverage   82.11%   82.11%
=======================================
  Files          17       17
  Lines         425      425
  Branches       72       77    +5
=======================================
  Hits          349      349
  Misses         72       72
  Partials        4        4

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:tada: This PR is included in version 1.0.1 :tada:
The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot :package::rocket:


[board-server] multi platform docker builds

Enable linux/arm64, linux/amd64 and darwin/arm64 builds to cover more bases
Remove all tags other than latest and semver tags, as there seems to be something wrong with the sha tagged images.

@Mearman can we check in on where you got up to with this?

@wfaithfull pipeline now actually tests the build.
you can see here where it was failing when trying to use darwin base image

Nature of this has somewhat changed from the original intent but @Mearman can you review & check if this all makes sense to you?


Adding view option on rack

Feature request.
Environment
NetBox version: 2.4.3
Proposed Functionality
Under rack view add the possibility to toggle between all/active/planned devices.
Use Case
When sending out fieldworkers to rack devices we would like to highlight the current equipment and what is planned to be added. In many cases a device can be listed as planned for weeks before its mounted. So it would be nice to be able to take out 2 views from netbox.
Excising and then how it should look like when done.

Marking this as blocked until we get SVG rack rendering in place (#2248).


Refactor flow.h's RX tracking to use a std::deque.

Also

Rename oldest_inorder_item and latest_inorder_item to cur_msg_train_head and cur_msg_train_tail
Rename RxTracking::Push to RxTracking::Add

LGTM.


2020mid

Please make sure to check the following boxes creating your pull request, thank you very much!

[x] Add Blog Header, including title, author, image, categories, tags, etc.
For example:
---
title: Your Great Blog Title
author: your_github_id
categories: project
tags:
  - python
  - padlocal
image: /assets/2021/02-your-blog-slug/teaser.jpg
---

One line bio

Your blog post content Your blog post content
Your blog post content Your blog post content
Your blog post content Your blog post content

It will be easier to start drafting your post by using a existing post file as your template, for example, this one

[x] Keep all filenames lowercase, and use - to connect words instead of space. e.g. 2017-10-06-wechat-pc-impactor.md instead of 2017-10-06-WeChat PC Impactor.md

[x] Embed video/pdf from YouTube/Bilibili/local (if there's any) by using {% include iframe.html src="URL_or_PATH" %}. (Learn more about how to embed file/url in the post by reading this blog post

[x] Save photo to local before publishing, to a folder named by ${MONTH}-${YOUR_BLOG_SLUG}/ under /assets/2021/ directory.

[x] Select a beautiful and meaningful teaser image for your blog

[x] Create your contributor profile (if you are a first time contributor)
For excample:

jekyll/_contributors/lijiarui.md

---
name: Jiarui LI (李佳芮)
site: https://lijiarui.github.io
bio: 句子互动创始人 & CEO，微软人工智能最具价值专家 (AI MVP)
avatar: /jekyll/assets/contributors/lijiarui/avatar.png
email<EMAIL_ADDRESS>twitter: https://weibo.com/u/2175505900
---

Here is the example file:

lijiarui.md: https://github.com/wechaty/wechaty.js.org/blob/master/jekyll/_contributors/lijiarui.md
It will shown like this on wechaty website: https://wechaty.js.org/contributors/lijiarui/

To learn more about the contribute guideline, visit: https://wechaty.js.org/docs/contributor-program/

You should not delete the following files, since they are unrelated with this PR and will damage other blogs:

jekyll/_posts/2021-06-05-summer-2021-open-source.md
jekyll/assets/2020/08-wechaty-soc-midterm-demo-day/midterm-demo-day.webp


Fix security issue by updating decompress-zip

Got a warning corresponding to this security issue:
https://www.npmjs.com/advisories/777
From the issue description:

"For decompress-zip 0.3.x upgrade to 0.3.2 or later."

Please update the decompress-zip to at least 0.3.2
Until then I have recommended on packages that use this to replace for a maintained package

@kevinsawicki You were the last person to commit to this repo. Is this issue something you can look into. asar depends on this package, which a LOT of other packages depend on. Should be a quick fix by someone that has push permissions.

Fixed from asar's POV in a major release, but not retroactively through this patch, such as<EMAIL_ADDRESS>Asar also wasn't the only dependent (small packages, to be fair).
I think its still worth doing the patch, and maybe a deprecation message. :smile:


how to set maximum is 99+

hello，
if number more then 99,i want to set maximum is 99+.

sorry, we don't support this but it should be relatively easy to implement


Document feature comparison with envy

After seeing envious in TWIR's "crate of the week" segment, I immediately thought of another, very similarly named library that does "the same thing" and that I've been using for my pet projects for a while now: https://github.com/softprops/envy. And I imagine I'm not the only one. Can we have a doc section (maybe not even in README, a separate file is fine) with a feature comparison?

Good idea!


vulnerabilityreports - not properly processed by fluentd

Describe the bug
fluentd fails or doesn't properly process the trivy vulnerabilityreports
To Reproduce
Steps to reproduce the behavior:

Login to elastic web interface
Filter the pods for scan-vulnerabilityreport
Check the message field

Expected behavior
Properly process the logs or ignore them for this pods
Screenshots

Version (add all relevant versions):

Compliant kubernetes apps version v0.17.0

Additional context
NA

seems solved now