create metadata file for empty detection results

detect_weeds.py is not creating a metadata file no detections. Regardless if the image has or does not have detection results, the metadata file should still be there.

@mkutu What should be included in this metadata file?
I have attached an example of metadata for detection.
ALA00138.json

use this file as a reference. What I changed:

renamed "exif_meta" to "exif_info"

reorganized image_info

added plant_field_info

turned category into a single dictionary

removed username

removed "MasterRefID"

removed basename

renamed detection_results to annotation

made annotation a dictionary

removed image_id from annotation dictionary

removed class_id from annotation dictionary

Exif_info should contain these keys only:
ImageWidth
ImageLength
Make
Model
Software
DateTime
ExposureTime
FNumber
ExposureProgram
ISOSpeedRatings
RecommendedExposureIndex
ExifVersion
BrightnessValue
MaxApertureValue
LightSource
Flash
FocalLength
ExposureMode
WhiteBalance
FocalLengthIn35mmFilm
Contrast
Saturation
Sharpness
LensModel
LensSpecification
BodySerialNumber

renamed bbox to bbox_xywh (bbox coordinates need to be changed accordingly). This should be a list of values xywh

ALA00138_updated.json

@mkutu Have you pushed these changes?

@navjot-nangia I only change the file itself. I haven't changed anything.

DONE

@navjot-nangia keep "category" even if there are not detection results
make bbox_xywh as null if there is not detection result.
"annotation": {
    "bbox_xywh": null
}

use this dataclass to format the metadata, and this method to extract it

make the internal "bbox" variable in segment.py "_bbox" and write a comment or 2 to document the fact that it's internal use only

After using the  method, these three parameters have issue:

exif_data: "ExifImageWidth". In the updated json that you shared above: "ImageWidth". Can i just rename it?
exif_data: "ExifImageLength". In the updated json that you shared above: "ImageLength". Can i just rename it?
I did not find any "BodySerialNumber" in the exif_data.

Other parameters are good.
The exif data from one of the images:  exif_data.txt

@navjot-nangia  yea you can just rename them. I think bodyserial number only is exposed in some camera for some reason. Looks good

DONE


Time Series Builder for COSI GRBs:

- Takes in a COSI binned dataset containing signal plus background before and after the GRB
- A response file (.hdf5 format or an OGIP compatible .rsp file)
- If response is .hdf5, requires an ori file and source location to correctly rotate the response and saves a .rsp file and
   .arf file for future use
- Creates a threeML compatible time series object which can be used in threeML to perform spectral fits

Hi @saurabhmittal23. Thanks for working on this! I'm sorry it took me a while, but I finally read your code in detail.
Although ultimately we want to use the full CDS for the GRB analysis, this code is useful because it will allow the user to do a simple ON/OFF analysis, which will be more familiar to many. It's also quicker, easier to visualize, and can be compared directly to other instruments.
I think that what we ultimately want to do is to add a function to 3ML itself, as suggested by the 3ML documentation:

Note: If you would like to build a time series from your own custom data, consider creating a TimeSeriesBuilder.from_your_data() class method.

I don't think we want to add the function to 3ML yet, because our data format is still in flux. You can however re-write your code so that it is a simple copy-past once we get to that point. You already have the code, it just needs to be refactored. Instead of creating a child class of TimeSeriesBuilder, we only need a single function, which you already have:
def from_cosi_grb_data(cls, *args, **kwargs):
    ...

This will be copy-pasted into 3ML's TimeSeriesBuilder (like all other missions), but for now it can be called with:
from threeML.utils.data_builders import TimeSeriesBuilder

cosi_data = from_cosi_grb_data(TimeSeriesBuilder, *args, **kwargs)

# Once from_cosi_grb_data is part of the 3ML code, the above is exactly the same as:
# cosi_data = TimeSeriesBuilder.from_cosi_grb_data(*args, **kwargs)

This is just using how Python knows about their own cls (for class methods) when called from the class itself, but otherwise, their class methods work like any other function.
In order to make this work, you need to move the code that you currently have in TimeSeriesBuilderCOSI.__init__() to from_cosi_grb_data() (right before your cls()) and instead of calling super(TimeSeriesBuilderCOSI, self).__init__() you'll use cls() (but this time cls = TimeSeriesBuilder). Everything else should be mostly the same.
Here are other comments besides this re-structuring:

I'd use the more generic name from_cosi_data, that is, dropping the GRB part, since this could be use for other transients.
I rather avoid defining another class for binned data, that is, COSIGRBData. Please use what's available for a generic binned data in DataIO. If there's something you need to achieve your goals that is not in DataIO, then feel free to modify that module. But again, it's something that can be used in general, not just GRBs.
Please add a simple example of how to use this. I suggest adding a jupyter notebook here.  Even better if you analyze one of the DC3 GRBs!
Please add unit tests for your new code. You can find some help here.
Eventually (this can moved to a future request issue), there should be the option to perform an ARM cut on the data. We really need this to reach something closer to the expected sensitivity of the instrument, even if it's a simple ON/OFF analysis.

Please don't hesitate to reach out if you need help with any of this.


adding google analytics tracker

Wondering if there is a way to test this to make sure it picks up the traffic.

Wondering if there is a way to test this to make sure it picks up the traffic.

not really, anything not coming from the right domain usually will be ignored by google analytics.


Fix tensors function for Operator type

Summary
This PR addresses a bug in the tensors function which caused a MethodError when called with a TensorNetwork of Operator type. The root cause was that the function signature was designed exclusively for the State type. The solution was to extend the function signature to handle both Operator and State types.
In addition to the fix, this PR introduces new tests designed to cover this scenario and prevent regressions.
Example
Here we can see an example of the previous error:
julia> _arrays = [rand(1, 1, 2, 2), rand(1, 1, 2, 2), rand(1, 1, 2, 2)]
3-element Vector{Array{Float64, 4}}:
 [0.5446558537456561;;; 0.43215020981519403;;;; 0.032683161122349036;;; 0.6659303187609796]
 [0.08054317169533542;;; 0.48062834256926257;;;; 0.28742074946598595;;; 0.17849508460251406]
 [0.7073563711310278;;; 0.12764406681391194;;;; 0.34387035001349886;;; 0.8791235663956318]

julia> ψ = MatrixProduct{Operator,Periodic}(_arrays, order = (:l, :r, :i, :o))
TensorNetwork{MatrixProduct{Operator, Periodic}, NamedTuple{(:plug, :interlayer, :χ), Tuple{Type{<:Plug}, Vector{Bijections.Bijection{Int64, Symbol}}, Union{Nothing, Int64}}}}(#tensors=3, #labels=9)

julia> tensors(ψ, 1)
ERROR: MethodError: no method matching tensors(::Type{Operator}, ::TensorNetwork{MatrixProduct{Operator, Periodic}, NamedTuple{(:plug, :interlayer, :χ), Tuple{Type{<:Plug}, Vector{Bijections.Bijection{Int64, Symbol}}, Union{Nothing, Int64}}}}, ::Int64)

Closest candidates are:
  tensors(::Type{Operator}, ::TensorNetwork{<:Quantum}, ::Any, ::Symbol)
   @ Tenet ~/.julia/packages/ValSplit/MMCz3/src/ValSplit.jl:141
  tensors(::Type{State}, ::TensorNetwork{<:Quantum}, ::Any)
   @ Tenet ~/git/Tenet.jl/src/Quantum/Quantum.jl:84
  tensors(::TensorNetwork{<:Quantum}, ::Integer, ::Any...)
   @ Tenet ~/git/Tenet.jl/src/Quantum/Quantum.jl:83

Stacktrace:
 [1] tensors(::TensorNetwork{MatrixProduct{Operator, Periodic}, NamedTuple{(:plug, :interlayer, :χ), Tuple{Type{<:Plug}, Vector{Bijections.Bijection{Int64, Symbol}}, Union{Nothing, Int64}}}}, ::Int64)
   @ Tenet ~/git/Tenet.jl/src/Quantum/Quantum.jl:83
 [2] top-level scope
   @ REPL[9]:1

There is a reason why tensors is only defined for States and not Operators, being that it is not as easy for Operators when the input and output sites do not match. This is the case of the Spaced Matrix Product Operator, for example.
I'm closing this PR without merging because I don't think this should be the solution, but thanks for reminding that I have yet to fix this.
@jofrevalles do you mind opening an issue?
If you need a quick fix, you can use the interlayer property of Quantum TNs along with the select function.

Okay. Since right now MPO and PEPO do only support the same amount of inputs/outputs I thought that was a good idea to implement that. Nevertheless, let's discuss that in an issue.


Dont split on a value that is already a list

Resolves https://github.com/juju-solutions/layer-filebeat/issues/6

+1


Sortable columns

Nice addition. I will merge once a few house keeping tasks are completed.
Can you:

Add a test for the new functionality. The test should be added to tableDynamicSpec.js
squish your commits into one with the commit msg:

feat(header.html): allow reordering of columns
Note: the ability to reorder columns is only available when using ng-table-dynamic

... oh and also I don't think the change to the .gitignore should be made


One bot reply in group chat from more than one account

Bug report
Description
Bot replies in group chat to individual commands (with botname) from several accounts.
main_acc: !command bot1
other_acc: <bot1> (message)
main_acc: <bot1> (message)

main_acc: !command bot2
other_acc: <bot2> (message)
main_acc: <bot2> (message)

Affected commands: status, owns, addlicense, play, resume.  Other commands not checked.
There are two accounts with ASF bots in group and one friend acc without. Bot reply from two accs linked to bots.  Bot1 linked to main_acc and bot2 to other_acc.
Expected behavior
Bot must reply from one account, which is linked in bot config:
main_acc: !command bot1
main_acc: <bot1> (message)

main_acc: !command bot2
other_acc: <bot2> (message)

Actual behavior
See description.
But when command sent to all bots, without botname, all happens correctly - every bot reply one time from only linked acc:
main_acc: !command
other_acc: <bot2> (message)
main_acc: <bot1> (message)

Steps to reproduce
Two bots set for two accounts.
Add accounts into group.
Enter command with botname in group chat.
Full log.txt recorded during reproducing the problem
Not logged anything about command processing, only about farming.
Global ASF.json config
ASF.json left default, changed only SteamOwnerID:
{
	"AutoRestart": true,
	"BackgroundGCPeriod": 0,
	"Blacklist": [],
	"CommandPrefix": "!",
	"ConfirmationsLimiterDelay": 10,
	"ConnectionTimeout": 60,
	"CurrentCulture": null,
	"Debug": false,
	"FarmingDelay": 15,
	"GiftsLimiterDelay": 1,
	"Headless": false,
	"IdleFarmingPeriod": 8,
	"InventoryLimiterDelay": 3,
	"IPC": false,
	"IPCPassword": null,
	"IPCPrefixes": [
		"http://<IP_ADDRESS>:1242/"
	],
	"LoginLimiterDelay": 10,
	"MaxFarmingTime": 10,
	"MaxTradeHoldDuration": 15,
	"OptimizationMode": 0,
	"Statistics": true,
	"SteamOwnerID": [removed],
	"SteamProtocols": 3,
	"UpdateChannel": 1,
	"UpdatePeriod": 24,
	"WebLimiterDelay": 200,
	"WebProxy": null,
	"WebProxyPassword": null,
	"WebProxyUsername": null
}

BotName.json config of all affected bot instances (if more than one)
{
  "Enabled": true,
  "StartOnLaunch" : true,
  "SteamLogin": [removed],
  "SteamPassword": [removed],
  "SteamMasterClanID" : [removed]
}

Other bot config is simular but other login&password of course.
Additional info
ASF release <IP_ADDRESS> for Win64
(8571cd25-39e6-4a02-9a3b-aabf31c62581)

https://github.com/JustArchi/ArchiSteamFarm/wiki/Commands#notes

Please note that sending a command to the group chat acts like a relay - if you're saying redeem X to 3 of your bots sitting together with you on the group chat, it'll result in the same as you'd say redeem X to every single one of them privately. In most cases this is not what you want, and instead you should use given bot command that is being sent to a single bot in private window. ASF supports group chat, as in many cases it can be useful source for communication with it, but you should almost never execute any command on the group chat if there are 2 or more ASF bots sitting there, unless you fully understand ASF behaviour written here and you in fact want to relay the same command to every single bot that is listening to you.
And even in this case you should use private chat with <Bots> syntax instead.

As @MikeLund stated above.

It not that case.
If i send command to bot through private channel, it expect that i communicate with bot linked to acc which private chat opened.
And for main account bot private chat not abailable.

Ok, it's feature (hardly to understand), not bug


Mark the test as flaky again

It seems that I was lucky to see 10 consecutive successes in #68511. Now the test seems to be quite flaky again.

Merge on red to fix the tree.


QUIC (curl+quiche) unable to match performance of TCP in simulated network

I am running some experiments comparing the performance of QUIC vs TCP in a simulated network, and wondering why QUIC is unable to match the performance of TCP, or at least be much closer. I am using curl compiled with quiche and sending either an HTTP/3 or HTTP/1.1 POST request with variable size payload (x-axis) to a webserver with one hop in between (Fig. 1). The near subpath has 100Mbps bandwidth and 2ms RTT, the far subpath has 10Mbps and 150ms RTT, and the egress queue at each host has a maximum size of 140 packets. The queue size was calculated to allow at least 1 BDP (152 milliseconds) * (10 megabits per second) / (1500 bytes) = 0.152 * 10000000 / 1500 / 8 = about 127.
The optimal tput based on the bottleneck link is 10Mbps = 1.25MB/s. TCP achieves this but QUIC is far below. I am trying to figure out why. I feel this difference is too large to explain by userspace overheads.
I am observing that QUIC sets a very conservative cwnd compared to TCP (Fig. 2). I plotted this graph by logging each time the cwnd changed in quiche code for a single 20MB transfer, and a TCP iperf3 connection for the equivalent amount of time. The optimal tput should be ~1 BDP = (127 packets) * (1500 bytes) = 127 * 1500 / 1000 = ~200 kBytes. The TCP cwnd hovers above and the QUIC cwnd much below.

Fig. 1 HTTP POST request tput (left) and Fig. 2 cwnd for a single 20M request (right)

Previously, I have also observed that while TCP pretty much always sends MTU-sized packets (~1500 bytes), QUIC often sends a little less. Maybe since the queue limit is based on number of packets rather than bytes this is artificially reducing the cwnd? But why wouldn't QUIC just send MTU-sized packets? Would love to hear any thoughts.

In these experiments, I also increased the MTU from 1200 to 1460 in quiche to be comparable to TCP.

Do you see similar behaviour when using the quiche-client application for an HTTP/3 upload?

In these experiments, I also increased the MTU from 1200 to 1460 in quiche to be comparable to TCP.

So you mean you changed curl? https://github.com/curl/curl/blob/fc8ad0b23c9fd922ebdb89348d467df037da1775/lib/vquic/curl_quiche.c#LL423C21-L423C21

@ygina can you describe your test environment a little more?

web server software and version (for tcp and quic, both)
client and server O/S
any noticeable difference between quiche default mtu, or 1460 mtu?
can you try to download with same size and see the difference?

Do you mean you changed curl?

Yup I added a line in curl here quiche_config_set_max_send_udp_payload_size(qs->cfg, 1460).

can you describe your test environment a little more?

Built with quiche 1aa5c702 and on curl curl-7_87_0 release from 2022-12-21.
The webserver is nginx built with quiche. It makes a proxy pass to a basic Python webserver that returns 200 OK to every request, because otherwise nginx would discard the payload. The nginx.conf configuration file looks like this:

events {
    worker_connections  1024;
}

http {
    server {
        # Enable QUIC and HTTP/3.
        listen 443 quic reuseport;

        # Enable HTTP/2 (optional).
        listen 443 ssl http2;

        ssl_certificate      <cert.pem>;
        ssl_certificate_key  <key.pem>;

        # Enable all TLS versions (TLSv1.3 is required for QUIC).
        ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;

        # Add Alt-Svc header to negotiate HTTP/3.
        add_header alt-svc 'h3=":443"; ma=86400';

        location = "/" {
            client_max_body_size 100M;
            client_body_buffer_size 100M;
            client_body_in_single_buffer on;
            client_body_temp_path /tmp/client_body_temp;

            proxy_pass http://<IP_ADDRESS>:8000;
        }
    }
}

OS is 22.04.1 LTS (Jammy Jellyfish), but running in mininet as described above.

any noticeable difference between quiche default mtu, or 1460 mtu?

This is only one trial but it looks like 1460 mtu is slightly better than default 1200.

Do you mean you changed curl?

Yup I added a line in curl here quiche_config_set_max_send_udp_payload_size(qs->cfg, 1460).

I guess you also need to update what @LPardue pointed in curl in addition to your change, because that code will limit the outgoing packet size, regardless of what you've set using quiche_config_set_max_send_udp_payload_size(). Probably there are other places to look -- trying with quiche-client would be helpful too.

I guess you also need to update what @LPardue pointed in curl in addition to your change, because that code will limit the outgoing packet size, regardless of what you've set using quiche_config_set_max_send_udp_payload_size(). Probably there are other places to look.

@junhochoi Oh yea, I'd changed that too: https://github.com/ygina/curl/blob/2masot-sidecar/lib/vquic/quiche.c#L549. Checked in Wireshark that the maximum packet sizes had increased.

Trying to upload with quiche-client/quiche-server would be helpful too.

It seems like the current quiche-server doesn't actually read the HTTP request payload (https://github.com/cloudflare/quiche/blob/master/apps/src/common.rs#L1555)? But I was able to use quiche-client to POST requests to the nginx server using the --body option:

Wonder what's happening at 10MB there...

10MB sounds like the example flow-control window size. What values does the server have configured, possibly defaults based on your nginx conf

@ygina please open a separate issue about POST handling on quiche-server, we should properly consume and drain the stream to help tests like this. Should be a straightforward change.

10MB sounds like the example flow-control window size. What values does the server have configured, possibly defaults based on your nginx conf

I'm not immediately sure what you mean by "example flow-control window size." Mind elaborating?

I should have been clearer sorry, the description in https://github.com/cloudflare/quiche/tree/master/nginx#http3_initial_max_data should help. Unless you specify something different in your configuration, you'll be adding flow control limit of 10 meg

It's looking a little better! At least, there is no sudden dropoff at 10MB.

I'm guessing because otherwise, if we exceed the per-connection flow control limit the server spends resources negotiating with the client over flow control limit increases? What does this process look like? quiche-client is represented as quic-quiche-<http3_initial_max_data>-<http3_initial_max_stream_data>. curl is represented by quic-1460.
And why is it that the default per-stream flow control limit is only 1m? I tried setting this to 100m (is that megabytes?) but the results were worse.
I reran the first experiment using curl and a flow control limit of 100m. Maybe it's a little better but still much worse than tcp:

I came across this fastly article describing the tricks they use to get quic's performance on par with tcp/tls. Wondering if it's necessary to say, implement the Delayed Ack extension, or to coalesce packets with GSO.

Flow control is intended to constrain local state commitment. Imagine a server that handles thousands of connections, they can't all have access to all of the memory. Flow control exerts back pressure. It's typically linked to how fast you can drain incoming data from the transport and pass it on.
Picking a Flow control size is an application and deployment choice. Being Conservative with defaults avoids nasty surprises.

@ygina what's your linux client/server's value of /proc/sys/net/ipv4/tcp_no_metrics_save? if 0, can you try with 1?

@LPardue If I understand the parameter correctly, setting http3_initial_max_data to 100m should be good in terms of space for this single connection setup while not needing to increase the per-connection limit. This change makes the dropoff from the 4th graph in this issue disappear, but quiche-client still settles into a throughput that is much lower than tcp's when the data size is larger. In Wireshark, even though many of the packets start out mtu-sized (1392 bytes here), at the tail end of the connection the packets become much smaller, which does not seem efficient for a large data transfer? In comparison, tcp basically only sends mtu-sized packets.

@junhochoi It was 0 before. I set it to 1, but the tcp line is the same.

ygina, if you run curl with QLOGDIR= it will generate some qlogs that you can zip up and share here and/or analyse via qvis https://qvis.quictools.info/#/files

Cool! Here's the qlog from a 20M transfer using curl: curl_20M.zip. I'm currently checking out the visualization.

I've been stuck on why quiche sends so many below-mtu sized packets and I narrowed it down to these lines. Essentially, when the remaining available cwnd is smaller than an mtu, quiche decides to send a packet anyway, but the per-packet processing overheads of quic are quite high. I replaced the lines with the following such that quiche waits for the available cwnd to grow so that, if it has enough data to send, it only sends mtu-sized packets:
let cwnd_available = self.paths
    .get(send_pid)?
    .recovery
    .cwnd_available()
    .saturating_sub(overhead)
    .saturating_sub(left_before_packing_ack_frame - left); /* bytes consumed by ACK frames */
if cwnd_available < left {
    left = 0;
}

I managed to get quic's performance to get much closer (satisfactorily close for me) to tcp using both curl and quiche-client (see below). Actually, curl outperforms quiche-client now!

Is this still to spec? A bug? Or some other reason for why these lines are the way they are currently?

I increased http3_initial_max_stream_data to 50m from 1m and now quic matches tcp. That makes me happy. Strange since that same configuration change hurt performance before the cwnd mtu fix.

Looks like that change is introduced in https://github.com/cloudflare/quiche/pull/1385.
@ygina can you try with 5a753c25e43e1d7b32df9d8c284fe0cab2b19cb4 (previous commit of this PR)? you'll need to rebuild clients only, not the server.

@junhochoi Yes, I tested with https://github.com/cloudflare/quiche/commit/5a753c25e43e1d7b32df9d8c284fe0cab2b19cb4 and the performance is the same as quic-quiche-1000m-1m. Probably because the changes in #1385 don't affect non-ACK packets that make up the majority?

I've been stuck on why quiche sends so many below-mtu sized packets and I narrowed it down to these lines. Essentially, when the remaining available cwnd is smaller than an mtu, quiche decides to send a packet anyway, but the per-packet processing overheads of quic are quite high. I replaced the lines with the following such that quiche waits for the available cwnd to grow so that, if it has enough data to send, it only sends mtu-sized packets (ygina@3e461b6):

I believe this is the problem but possible it's not the fix. This will affect anything that has to do with number of packets, such as queue sizes limited by packet counts as opposed to bytes, and congestion control algorithms that operate in terms of packet counts.

There's an inherent tradeoff between thoughput and latency.
A sender that holds off from emitting a packet in order to either fill an MTU-sized packet, or wait for the CWND to grow (via receiving the peer's ACK) can introduce additional latency. If the sending packet itself contains an ACK for the peer, which it would use to control its CWND, then things get a bit weird.
This all sounds a bit like https://en.wikipedia.org/wiki/Nagle's_algorithm
There's possibility that tuning could improve things. But we have to be aware different applications that use QUIC will have different needs to trade off.

(Hi all, I'm working with @ygina on this.)

A sender that holds off from emitting a packet in order to either fill an MTU-sized packet, or wait for the CWND to grow (via receiving the peer's ACK) can introduce additional latency.

I'm a bit confused by this. TCP is often implemented (e.g. in Linux) with a cwnd maintained in MSS-sized increments, so this problem basically doesn't occur. It's true that if a flow is application-limited, you do have to decide whether to "hold off" from emitting a partial segment now vs. waiting for the user to supply more data. This is what Nagle's algorithm is meant to address.
But here the flow isn't application limited; the application is trying to send tens of megabytes that's all queued up already, and waiting for room in the cwnd to transmit. So I think the only decision is about whether to send a lot of small packets (e.g. one small packet now, and then more small packets when some data is ACKed or cwnd grows), vs. sending MTU-sized packets as long as the flow remains non-application-limited. This seems like more of a "silly window syndrome" issue than something related to Nagle's algorithm.

If the sending packet itself contains an ACK for the peer, which it would use to control its CWND, then things get a bit weird.

My understanding had been that in both TCP and QUIC, ACK-only packets can be sent at any time, irrespective of cwnd -- is that right? If so I don't think I see the problem.

let cwnd_available = self.paths
    .get(send_pid)?
    .recovery
    .cwnd_available()
    .saturating_sub(overhead)
    .saturating_sub(left_before_packing_ack_frame - left); /* bytes consumed by ACK frames */
if cwnd_available < left {
    left = 0;
}

What if we check that if both cwnd_available < left (the remaining cwnd is less than an MTU) and the remaining amount of data to send exceeds cwnd_available, set left = 0? That is, wait for room in the cwnd to transmit if the flow remains non-application-limited. Otherwise set left = cmp::min(cwnd_available, left).

Hi @keithw, appreciate the comments. Lots of possible things to discuss here :smile: Junho can probaly weigh in too

TCP is often implemented (e.g. in Linux) with a cwnd maintained in MSS-sized increments, so this problem basically doesn't occur.

Does that apply to all congestion control algorithms?
Generally the best way for quiche to work is to send a flight of paced packets. Assuming we are not app limited, we'd ideally use something like GSO to send a full CWND's worth of data, and then wait for the ACKs to come back so we can repeat. Reading a complete set of ACKs before attempting to send again will mean that the entire CWND is available to use.

My understanding had been that in both TCP and QUIC, ACK-only packets can be sent at any time, irrespective of cwnd -- is that right? If so I don't think I see the problem.

QUIC packets that contain only ACKs do not count towards CWND. But there's no requirement for them to be sent that way. Sending small, ack only-packets adds overhead in the form of packet construction, encryption and syscalls. But they could improve responsiveness. Deciding what is a right hit is hard! Some implementations might prefer to bundle the ACKs with other frame types to amortize the costs; techniques like GSO rely on slicing data into same-sized payloads, with optionally one smaller overflow at the end. In a case where there is high rate of data exchange in both directions, batching can be a useful optimization. Sending even more ack-only packets separately is entirely possible, but would have to be weighed against batching. Also it sounds like it wouldn't help in a scenario like yours that is counting packets. But I don't know how common that is.
Unlike TCP, QUIC has multistreaming and in-band control frames. In situations where the timely delivery of something like a RESET_STREAM, MAX_STREAM_DATA frame, or PATH_VALIDATION frame is important, then not sending those things when there is sufficient CWND to do so could be harmful. Mostly that's not a problem if they get batched in with the application data though, they should go out in the first packet in our flight of packets model.
At the application level, app performance can be enhanced by sending data for different concurrent streams even when not app limited and cwnd < mtu. For instance, dispatching HTTP requests ASAP, rather than waiting for cwnd to grow to MTU size.
I'm not saying everything in quiche is perfectly designed. There may be improvements or ways to expose tuning to apps so they can use the connection having made their own tradeoff decisions. So discussions like this are really useful, thanks.

Hi @keithw, appreciate the comments. Lots of possible things to discuss here smile Junho can probaly weigh in too

TCP is often implemented (e.g. in Linux) with a cwnd maintained in MSS-sized increments, so this problem basically doesn't occur.

Does that apply to all congestion control algorithms?

On Linux, yes, that's my understanding. The individual congestion-control algorithms adjust cwnd in different ways, but the logic that interprets cwnd is part of the common tcp.h and tcp_output.c, and it's interpreted as a packet count. See https://github.com/torvalds/linux/blob/master/net/ipv4/tcp_output.c#L2714 and https://github.com/torvalds/linux/blob/master/include/net/tcp.h#L1204-L1228, as well as RFC 5681:

   The RECOMMENDED way to increase cwnd during congestion avoidance is
   to count the number of bytes that have been acknowledged by ACKs for
   new data.  (A drawback of this implementation is that it requires
   maintaining an additional state variable.)  When the number of bytes
   acknowledged reaches cwnd, then cwnd can be incremented by up to SMSS
   bytes.

I do wonder what other QUIC CUBIC implementations do (I haven't looked myself yet).

Generally the best way for quiche to work is to send a flight of paced packets. Assuming we are not app limited, we'd ideally use something like GSO to send a full CWND's worth of data, and then wait for the ACKs to come back so we can repeat. Reading a complete set of ACKs before attempting to send again will mean that the entire CWND is available to use.

Hmm, I'm not sure this is so desirable for a long-running bulk flow. Generally speaking, I think one would expect the ACKs to come back pretty continuously (in TCP, for every 2 segments delivered) as data-carrying segments are delivered to the receiver at the bottleneck link rate. The sender probably should be keeping the window close to full by sending more segments almost as soon as space opens up in the cwnd (i.e., even with a constant cwnd, the sender is transmitting one MTU-sized segment for every MTU-sized segment that gets ACKed). I may have misunderstood you; I don't think you'd want the sender to wait for a "complete set" of ACKs back (letting packets in flight go to zero and the cwnd open up completely?) before attempting to send anything again. The bottleneck buffer would go empty in that situation and you'd be wasting the link for some period of time. I'm sure GSO is useful, but hopefully it's being done on smaller units than "an entire RTT's worth of packets."
In any event, advisable or not, it doesn't seem to be what @ygina was observing in these long-running uploads to nginx -- I think she's seeing quiche sending packets of ~400 bytes or so.

My understanding had been that in both TCP and QUIC, ACK-only packets can be sent at any time, irrespective of cwnd -- is that right? If so I don't think I see the problem.

QUIC packets that contain only ACKs do not count towards CWND. But there's no requirement for them to be sent that way. Sending small, ack only-packets adds overhead in the form of packet construction, encryption and syscalls.

Sure, but... I'm a little confused why this would be a concern in the first place. AFAIK, in the case of a full-throttle flow, the data sender basically never has a need or desire to send a small ACK-only packet:

With a constant cwnd, the sender has an opportunity to transmit an entire MTU-sized segment every time it gets an ACK for a previous MTU-sized segment, just because that much room has opened up in the window.
If cwnd is increasing, e.g. in the "congestion avoidance" / additive-increase phase of a traditional congestion-control algorithm, the sender can still send strictly MTU-sized segments; the easiest way seems to involve representing cwnd as a count of MTU-sized segments, as in the RFC 5681 recommendation and Linux TCP implementation. Then basically for every MTU-sized segment that gets ACKed, the sender transmits either 1 or 2 MTU-sized segments in reply. It's never sending an ACK-only segment.

At the application level, app performance can be enhanced by sending data for different concurrent streams even when not app limited and cwnd < mtu. For instance, dispatching HTTP requests ASAP, rather than waiting for cwnd to grow to MTU size.

I think I'm a little confused about the concern about "waiting for cwnd to grow to MTU size." If you're always sending MTU-sized segments, then room in the window is always going to open up in MTU-sized segments anyway, and it doesn't really matter whether cwnd growth is done gradually or in MTU-sized units. So I don't think you really have to worry that the sender will just be sitting around waiting for the window to open gradually byte-by-byte until it reaches 1 MTU of available room (or for cwnd itself to grow gradually byte-by-byte), even if quiche did things the Linux TCP / RFC 5681 way. (I mean Linux TCP is pretty widely used...)
What do you think is the right way to proceed here? If you think helpful, we could try to submit a pull request to quiche (along the lines of @ygina's proposal in https://github.com/cloudflare/quiche/issues/1400#issuecomment-1414520725). The broader context is that we're trying to write a paper that includes measurements of QUIC CUBIC (represented by quiche) and TCP CUBIC (represented by Linux) over the same emulated paths, and we'd like to benchmark each one as fairly as possible. I don't think it makes sense for us to show implausibly bad results for QUIC (because I don't think that's realistic or useful for the reader) but it also doesn't really seem right to make a substantial change locally that we weren't able to persuade the broader quiche community to accept, and then report this as representing "QUIC". :-/

I may have misunderstood you; I don't think you'd want the sender to wait for a "complete set" of ACKs back (letting packets in flight go to zero and the cwnd open up completely?) before attempting to send anything again. The bottleneck buffer would go empty in that situation and you'd be wasting the link for some period of time. I'm sure GSO is useful, but hopefully it's being done on smaller units than "an entire RTT's worth of packets."

I agree. The approach hinges on batching and pacing. No point blasting a large number of packets and waiting on them. So picking the right number to to send is critical. By offloading the UDP send parts to the kernel, the application is able to read incoming UDP and decide when to prepare the next batch.
I'm a bit rusty on the curl code but I don't believe its upload behaviour is very advanced compared to what is possible.

In any event, advisable or not, it doesn't seem to be what @ygina was observing in these long-running uploads to nginx -- I think she's seeing quiche sending packets of ~400 bytes or so.

Agree. The qlog does seem a bit odd. We see those regular small data-bearing packets. And from the client's perspective, the server acks are getting lagged way behind the packets the server is sending. Not sure if there is some impedance between the event loop happening there. How does the download case behave for a similarly sized file?
There's also batches of STREAM_DATA_BLOCKED with the same limit value, which I thought we'd fixed.

What do you think is the right way to proceed here? If you think helpful, we could try to submit a pull request to quiche (probably along the lines of @ygina's proposal in https://github.com/cloudflare/quiche/issues/1400#issuecomment-1414520725 ?).

I think some more investigation into the matter will help us better decide. There's some clues to look into further.

The broader context is that we're trying to write a paper that includes measurements of QUIC CUBIC (represented by quiche) and TCP CUBIC (represented by Linux) over the same emulated paths, and we'd like to benchmark each one as fairly as possible. I don't think it makes sense for us to show implausibly bad results for QUIC (because I don't think that's realistic or useful for the reader) but it also doesn't really seem right to make a substantial change locally that we weren't able to persuade the broader quiche community to accept, and then report this as representing "QUIC". :-/

Your research sounds interesting! And trying to give fair representation to each sounds great. We want these protocols to perform as close to each other as possible. I think investigating internal and external factors that might be contributing to the observations will make it clearer what to do next. And we can help with that.

Having read all this (many thanks for your help, @LPardue !) I wanted to add my 2 cents - as I stumbled over this paragraph and wanted to make sure it doesn't reflect a misunderstanding:

Unlike TCP, QUIC has multistreaming and in-band control frames. In situations where the timely delivery of something like a RESET_STREAM, MAX_STREAM_DATA frame, or PATH_VALIDATION frame is important, then not sending those things when there is sufficient CWND to do so could be harmful. Mostly that's not a problem if they get batched in with the application data though, they should go out in the first packet in our flight of packets model.

Just to be clear - yes these messages should be sent, but our concern here isn't that the payload per packet might fluctuate - @ygina (as was mentioned with the 400 byte packets) really sees the entire packet being small, in Wireshark, not the payload per packet. That just shouldn't happen, I believe, when there's a large enough buffer of data that wants to get out.

Agree that the case of saturated upload is straightforward but demonstrating some oddness. I'd like to understand the root causes better in order to inform our decision about changes in the library. Since these are controlled conditions, it doesn't feel like an impossible task to figure it out. Appreciate the work you're doing and data that is being shared.

It'd be good to have a same trace for downloading same sized file to see if it has a same pattern (small packets) for download. If so, this could be upload-specific. Because I don't think quiche has similar issue with download.

Hello, I encountered some problems when configuring the quic service of NGINX. My configuration is the same as that of the official website, but I still cannot use the quic protocol when accessing the server. The h2 protocol is still used. Here is my compilation information:
server {
# Enable QUIC and HTTP/3.
listen 443 quic reuseport;
server_name  test.cn;
    # Enable HTTP/2 (optional).
    listen 443 ssl http2;

    ssl_certificate      /usr/local/nginx/conf/cert/test.pem;
    ssl_certificate_key  /usr/local/nginx/conf/cert/test.key;

    # Enable all TLS versions (TLSv1.3 is required for QUIC).
    ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;

    # Add Alt-Svc header to negotiate HTTP/3.
    add_header alt-svc 'h3=":443"; ma=86400';

}

Can you help me?

@Ryenum That looks similar to my configuration above. How are you accessing the server? Are you sure your client is using the quic protocol?

@ygina

Your drawings are very good. How did you draw them?

@pplabs-fute I just use matplotlib. :)


Fastbin Misalignment - Ubuntu 22.04 - Ros Humble

Hello,
My state machine is intermittently throwing a "malloc(): unaligned fastbin chunk detected" error. The address sanitizer reveals that there is a new-delete-type-mistmatch during the constructor of the state machine. This may have to do with the version of boost the smacc2 is designed to compile against. Can you please tell me which version of boost you use for smacc2?
Best,
Calen
┆Issue is synchronized with this Jira Task by Unito

Solved. Was related to client behavior being run from superstate.


tracklist.add by uris is broken

The uris param for tracklist.add is broken as it escapes the string too many times.
E.G. If I create a "mopidy out" tracklist.add node with the uris param === "spotify:playlist:37i9dQZF1DXaImRpG7HXqp" Mopidy logs errors with
ValidationError: Expected a list of URIs, not \'spotify:playlist:37i9dQZF1DXaImRpG7HXqp\'\\n"}}}'

If I change the uris param to ["spotify:playlist:37i9dQZF1DXaImRpG7HXqp"] Mopidy logs errors with:
ValidationError: Expected a list of URIs, not \'[\\"spotify:playlist:37i9dQZF1DXaImRpG7HXqp\\"]\'\\n"}}}'

I have a workaround for this. Create a tracklist.add node with the params you want. Then export that node only to clipboard. Edit the params section so it looks like:
"params": "{\"tracks\":\"\",\"at_position\":\"\",\"uris\":[\"spotify:playlist:37i9dQZF1DXaImRpG7HXqp\"]}",

And then import it back into Node Red.
After you have done this DO NOT EDIT the node in node red because it will undo your manual change.

Additionally if you are struggling with this, you can craft your own HTTP request to Mopidy like so:
[
    {
        "id": "67d43351.f64abc",
        "type": "function",
        "z": "a0f2f980.6ed158",
        "name": "",
        "func": "return {\n    headers: {\n        \"Content-Type\": \"application/json\",\n    },\n    payload: {\n        method: \"core.tracklist.add\",\n        params: {\n            uris: [\"spotify:playlist:37i9dQZF1DXaImRpG7HXqp\"]\n        },\n        jsonrpc: \"2.0\",\n        id: 1762,\n        event: \"websocket:outgoingMessage\"\n    }\n}",
        "outputs": 1,
        "noerr": 0,
        "x": 990,
        "y": 120,
        "wires": [
            [
                "5da811d1.d708b"
            ]
        ]
    },
    {
        "id": "5da811d1.d708b",
        "type": "http request",
        "z": "a0f2f980.6ed158",
        "name": "",
        "method": "POST",
        "ret": "txt",
        "paytoqs": false,
        "url": "http://localhost:6680/mopidy/rpc",
        "tls": "",
        "persist": false,
        "proxy": "",
        "authType": "",
        "x": 1150,
        "y": 60,
        "wires": [
            []
        ]
    }
]

Thanks for posting this!
I know this is old, but just wanted to share that I got it working!
Since there isn't a way to move a playlist into a tracklist, I first use `playlist.getitem - then pass that to a function node with the following:
let data = msg.payload;
let i = 1;
let arr = [];
msg.params = [];

while(data[i] != null) {
  arr.push(data[i].uri);
  i++;
}

msg = {
        host: '<IP_ADDRESS>',
        port: 6680,
        method:'tracklist.add',
        params: {uris:arr}
        };

return msg;

Then I pass this to the output node, and it seems to work!
If you are still using this, or if anyone else stumbles upon it like me - this solution should be working!
I tried to put a uri directly in the output node, but I haven't gotten that working... using a function node does seem to work though!
DeadEnd


Operator spacing should be less strict

Surprisingly, all of the following are valid syntax:
A [] // Array<A>
{ a: A } ['a'] // A
keyof{ a: 1 } // 'a'
a&b // a & b
a|b // a | b

First steps: https://github.com/samhh/tshm/commit/3b00b656469e153519a7b3139047ef4633c80acf
Each case needs verifying against TypeScript's behaviour. Note also that the postfix operators ([], ['k']) break if there's a space between them - check that in TypeScript in-particular. I have a feeling it's because of how I hacked on the "multi" code before.

First steps: https://github.com/samhh/tshm/commit/3b00b656469e153519a7b3139047ef4633c80acf
Each case needs verifying against TypeScript's behaviour. Note also that the postfix operators ([], ['k']) break if there's a space between them - check that in TypeScript in-particular. I have a feeling it's because of how I hacked on the "multi" code before.

Lots of commits today have fixed all of these examples, and then some.

Lots of commits today have fixed all of these examples, and then some.


Tap support

Allows you to tap modifier keys, for example to "tap alt(er)" to show the
window menu, or "tap super" to show the OS menu (eg Start Menu)

I'm happy with this PR, but since it adds a new global command I'd like input from some other maintainers - @pokey @splondike thoughts?
In particular, is tap the right word here? One alternative would be press. Another would be to have no verb at all and just allow saying super or control to press those keys -- unfortunately alt and odd are a bit similar so I think this is a bad idea to do by default. So I think tap is fine; any other opinions?
If nobody objects, I'll merge this in a few days.

Seems fine to me. I think I would prefer an explicit command rather than just saying the key name, but maybe just because it's something I don't do often. My only reservation about using tap is I can imagine some occasional overlap with trap (a least on my set up I'm sure that will happen), in that sense I would opt for press.

@SCdF can you change tap to press? If you do then I will merge this.


Unknown type error

Hi
I'm trying to connect with my Keystone JS app.
I need to use a mutation to make a login and get the user's token.
This is login.gql:
type Mutation {
authenticateUserWithPassword(email: String!, password: String!): UserAuthenticationWithPasswordResult!
}
type Query {
authenticatedItem: AuthenticatedItem
}
union AuthenticatedItem = User
union UserAuthenticationWithPasswordResult = UserAuthenticationWithPasswordSuccess | UserAuthenticationWithPasswordFailure
type UserAuthenticationWithPasswordSuccess {
sessionToken: String!
item: User!
}
type UserAuthenticationWithPasswordFailure {
message: String!
}
mutation {
authenticateUserWithPassword(
email<EMAIL_ADDRESS>password: "1234567890"
) {
... on UserAuthenticationWithPasswordSuccess {
item {
id
name
email
}
}
... on UserAuthenticationWithPasswordFailure {
message
}
}
}
I get those error when I run npm run dev:
Unknown type "UserAuthenticationWithPasswordSuccess"
Unknown type "UserAuthenticationWithPasswordFailure"
Do I doing something wrong?
Thanks

@yalondpsi A few points to take note of, the type definitions exist on the GraphQL API's schema  and hence shouldn't be manually added to your query / mutation definitions.

I get those error when I run npm run dev:
Unknown type "UserAuthenticationWithPasswordSuccess"
Unknown type "UserAuthenticationWithPasswordFailure"

These errors indicates that your Keystone system hasn't been extended to support authentication, you need to use the createAuth function to do so, ie: KeyStone JS Authentication Docs
Once you've utilized createAuth, your GraphQL schema will then have the type necessary for authentication, at which point your login query would be as seen below:
login.gql
mutation Login ($email: String!, $password: String!) {
  authenticateUserWithPassword(email: $email, password: $password) {
    ... on UserAuthenticationWithPasswordSuccess {
      item {
        id
        name
        email
      }
    }
    ... on UserAuthenticationWithPasswordFailure {
      message
    }
  }
}

Usage

<script lang="ts" setup>
async function handleLogin() {
  const result = await GqlLogin({
    email<EMAIL_ADDRESS>    password: '1234567890'
  })

  console.log('handleLogin', result)
}
</script>

@Diizzayy Hi
Thank you very much for the detailed answer.
I will try it

Working :)
I also added the 'sessionToken' to return a token from the mutation function Login.
Maybe the next question is not in the scope of the support of this module, but can you please tell me or refer me:
Where and how I store the token - to use it with the next gql calls to the api?
Thank you !!

Where and how I store the token - to use it with the next gql calls to the api?

This module provides a useGqlToken composable which can be used to attach your Auth Token to subsequent GraphQL requests.
<script lang="ts" setup>
async function handleLogin () {
  const result = await GqlLogin({
    email<EMAIL_ADDRESS>    password: '1234567890'
  })

  if (!result) { return }

  // apply token to future requests
  useGqlToken(result.login.sessionToken)
}
</script>

Where and how I store the token

You'll have to decide between storing your token in either a cookie ( my recommendation ) or in local storage based on your app's requirements (SSR etc), after doing so you can then create a nuxt plugin that will retrieve the token ( from cookie / local storage ) then apply it using useGqlToken.

Below is an example retrieving a token stored using cookie and applying it in your app.

ps: Have a look at the KeyStoneJS documentation on Authentication and Sessions
nuxt-app/plugins/init.ts
export default defineNuxtPlugin(() => {
  const token = useCookie('<cookie_name>')

  if (token.value) { useGqlToken(token.value) }
})

@Diizzayy Wow
Thank you again! much appriciated!!!

Hi @Diizzayy
I think there is a bug, not sure:
when I run yarn dev of the nuxt 3 app (latest rc 4) and the graphql api is down, I get an error:
Cannot start nuxt:  Failed to load schema for "gql-sdk.ts"
Failed to load schema from http://localhost:3000/api/graphql:
Unexpected token < in JSON at position 0
SyntaxError: Unexpected token < in JSON at position 0
Thank you

when I npm run dev of the nuxt 3 app (latest rc 4) and the graphql api is down, I get an error:

@yalondpsi This module currently requires an active GraphQL api ( for Code Generation )

@Diizzayy understood

@Diizzayy -

This module currently requires an active GraphQL api (for Code Generation)

are you saying that this will not work if your graphql api is being served by the nuxt app itself?
i too am seeing the error:

Unexpected token < in JSON at position 0

is the solution to save my introspection to json, then reference that?
in my nuxt.config.ts, if i add the following:
  'graphql-client': {
    codegen: false
  },

then it will run, however, in my setup function
  async setup () {
    const { data } = (await useAsyncGql({
      operation: 'AllThings'
    }))
    const things = data._rawValue.allThings.nodes
    return {
      things
    }
  }

i have to use the _rawValue to get to my actual data - is this the effect of no codegen for me?


Feature request: Start new sessions in ndb terminal

I love that ndb provides a CLI — it opens the door to ndb being a full JavaScript IDE! I think it can be tweaked to be even more useful than it already is.
Steps to reproduce
Tell us about your environment:

ndb version: 1.0.6
Platform / OS version: OS X 10.13.6
Node.js version: v8.9.4

What steps will reproduce the problem?
In ndb's terminal, run exit. The session is ended and cannot be restarted without reloading ndb.
Please include code that reproduces the issue.

exit, or ctrl+d

What is the expected result?
I can start a new shell session either manually or automatically.
What happens instead?
I have to restart ndb to get a new  session.

fixed only on mac.

<EMAIL_ADDRESS>is working perfectly for me. Thanks so much for the quick response and implementation @ak239!


[RFE] Cover the OpenTracing to OpenTelemetry migration path

@brunobat from the Quarkus team is working on a comprehensive guide to document the migration from OpenTracing to OpenTelemetry in the context of Quarks 3 applications. @dymurray I think this could be a good candidate for our team to try to produce rules based on documentation about a given migration path. Most if not all of the contents in the guide refer to Quarkus specifics, but I think it would also be interesting to try to figure out if we can extract some generic guidelines that could apply to other runtimes like Spring boot (there is a generic migration guide here), and then provide the runtime specific recommendations based on the source technology of the application.

The migration guide is now live in the following link: https://quarkus.io/version/main/guides/telemetry-opentracing-to-otel-tutorial


Linting

Refs #12

There's one pending coding standard error which I wasn't sure how to take care of, so please look into that.

The coding standards used is PSR2 which is the widely accepted coding style guide for PHP.

Group use statements

Yes it's a PSR2 requirement "There MUST be one use keyword per declaration."
Having one use declaration per line has it's benefits. For e.g. the Atom editor has a plugin which auto adds use statement at top of file when you use new Foo. I wish one day a vscode plugin would do the same :)

The line limit of 80 characters is just straight from the 90s. We all have 16:9 monitors

It's not 80, PSR2 has a soft limit of 120 characters.

Is there a way to ignore the space errors in the definition locations in the tests so they can stay table-like?

Yes you have use // @ignoreStandardsStart and // @ignoreStandardsEnd tag to make phpcs ignore violations in code lines between those tags. It have used it at couple of places in tests as I didn't want to go through the trouble of manually breaking long lines to under the 120 lines limit :)
Most of formatting changes were done automatically using phpcbf which comes along with phpcs.

Again you can configure phpcs with an XML file, so that would be appreciated.

Yes, I'll add it.

Current coverage is 85.69% (diff: 61.11%)

Merging #107 into master will decrease coverage by 1.29%

@@             master       #107   diff @@
==========================================
  Files            31         31
  Lines           761        769     +8
  Methods          99         99
  Messages          0          0
  Branches          0          0
==========================================
- Hits            662        659     -3
- Misses           99        110    +11
  Partials          0          0

Diff Coverage
File Path

0%
src/Project.php

0%
src/NodeVisitor/VariableReferencesCollector.php

•••••      55%
src/LanguageServer.php

•••••      56%
src/Fqn.php

•••••••••• 100%
src/Formatter.php

•••••••••• 100%
src/Client/Window.php

•••••••••• 100%
src/utils.php

•••••••••• 100%
src/Server/TextDocument.php

•••••••••• 100%
src/Protocol/Range.php

•••••••••• 100%
src/Protocol/Diagnostic.php

Review all 19 files changed

Powered by Codecov. Last update a19d225...ddeda49

I have added phpcs.xml.dist. You can see a good example of how you can customize the rules here.

Having one use per declaration also allows sniffers to easily check for unused declarations.

I don't see why group use statements wouldn't let the tools do this, at least for this LS I plan to fully support them. It's a new feature so it may not be widely supported but it makes the code so much more readable, and that's what linting is all about, checking that code style is consistent so it's easier to read 😄

It looks that for now this behavior wont be changed (PHP_CodeSniffer #878) because PSR2 standard is fixed and not adjusted to PHP 7. I think reasonable solution would be to exclude this rule with configuration file.

You guys can follow the coding style that suits you :)
You can just take the updated .travis.yml and new phpcs.xml.dist from this PR, update the latter as per your needs and then use phpcbf to autofix the coding style violations.

@ADmad will do 😉 Thanks for your help

No problem, really appreciate the work you are doing to make PHP language support awesome in vscode.

I have undone all code changes.

When this PR will be merged I can prepare next with adjusted config file to exclude things like use statements rule (and others if we will need).

Thanks a lot @ADmad!


[Question] Include own VCF files

Hi @ypriverol ,
Short question: Is it also possible to directly create a FASTA file based on your own VCF files using this pipeline?
Thanks a lot!
Johannes

Replace by https://github.com/nf-core/pgdb/issues/7


support ecto >= 2

As everyone is trying to upgrade to ecto 3, I changed the requirements to be >= 2.0.0 instead of lock it on ecto2.

Thank you!

Any idea when this might go out in a released version?  I see another commit on master has {:absinthe, "~> 1.5.0-alpha.0"}, so maybe this change is caught up in a release cycle for larger changes?

I opened #129 to backport this to the v1.4 release branch.


Allow underscore assignments in node containers

Description
Avoid trigger warning for underscore assignments in node containers; e.g.
Describe("example", func() {
    _ = funcReturnSomeValueEGError()
})

Type of change
Please delete options that are not relevant.

[x] 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)
[ ] This change requires a documentation update

How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[x] Added test case and related test data
[x] Update the functional test

Checklist:

[x] I have performed a self-review of my own code
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[x] Any dependent changes have been merged and published in downstream modules
[x] I ran golangci-lint

Pull Request Test Coverage Report for Build<PHONE_NUMBER>
Details

0 of 0   changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 64.396%

Totals

Change from base Build<PHONE_NUMBER>:
0.0%

Covered Lines:
208

Relevant Lines:
323

💛  - Coveralls


Update GitHub Desktop editor list

Why:
Closes:

What's being changed (if available, include any code snippets, screenshots, or gifs):

Check off the following:

[ ] I have reviewed my changes in staging, available via the View deployment link in this PR's timeline.

For content changes, you will also see an automatically generated comment with links directly to pages you've modified. The comment won't appear if your PR only edits files in the data directory.

[ ] For content changes, I have completed the self-review checklist.

@devlynnx Thanks for opening an PR! Could you fill in the PR template above and give us a bit more details as to why you are suggesting these changes?
Thank you for your interest in improving GitHub Docs! :sparkling_heart:

Thanks


ci: disable Fedora 35 as it is EOL as of 12-13-2022

Pull Request Test Coverage Report for Build<PHONE_NUMBER>

0 of 0   changed or added relevant lines in 0 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 65.535%

Files with Coverage Reduction
New Missed Lines
%

libraries/plugins/xfpga/mmio.c
2
84.72%

Totals

Change from base Build<PHONE_NUMBER>:
0.02%

Covered Lines:
13141

Relevant Lines:
20052

💛  - Coveralls

Pull Request Test Coverage Report for Build<PHONE_NUMBER>
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0   changed or added relevant lines in 0 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.03%) to 65.535%

Files with Coverage Reduction
New Missed Lines
%

libraries/plugins/xfpga/mmio.c
2
84.72%

Totals

Change from base Build<PHONE_NUMBER>:
0.03%

Covered Lines:
13141

Relevant Lines:
20052

💛  - Coveralls


Distributed Training of the Decoder

The same methods used in #112 are used here as well. Implementation is via huggingface accelerate to minimize changes needed to have a script capable of single or multi GPU training.
The method used was to convert the decoder trainer class to wrap its models with an accelerator object passed in as an argument. Functionality remains mostly unchanged. One new change was made to separate the loss values for the different unets during training and validation in order to make it easier to see which need more training.
This is not ready due to some remaining issues, but I wanted a place to track them and have others provide input.
Known Issues:

[x] Evalaution metrics hang on distributed mode
[x] Training loss drops slowly and arbitrarily diverges (This appears to also be an issue on the main branch)
[x] Loading the model causes the optimizer to error

Roadmap:

[x] Implement distributed DecoderTrainer
[x] Update train_decoder.py to use the distributed trainer.
[x] Fix remaining issues
[x] Verify functionality on large training set
[ ] Port script for imagen

Evalaution metrics hang on distributed mode

If you haven't already figured this one out, you might need to unwrap the model before doing evaluation. Not doing so can cause a deadlock.

Loading the model causes the optimizer to error

Could be an issue with param groups, I had a lot of issues with loading the optimizer. 1ed0f9d80b097594e6294bad432d06ce3fdcb220 and if you can, try using group_wd_params=False, it seems to work for the prior, at least. (Note: we can try using an OrderedDict for grouping the wd_params, I haven't looked into it, but it may be more deterministic than the standard dictionary.

The hanging on evaluation problem was caused by torchmetrics assuming evaluation is happening on all processes while I was only doing it on master. Using torchmetrics built in distributed mode causes errors with further dataloading, however. So one rabbit hole later I found that it was a problem with how torchmetrics does its gather and substituting my own gather using accelerate fixes the issue.

Could be an issue with param groups, I had a lot of issues with loading the optimizer. 1ed0f9d and if you can, try using group_wd_params=False, it seems to work for the prior, at least. (Note: we can try using an OrderedDict for grouping the wd_params, I haven't looked into it, but it may be more deterministic than the standard dictionary.

@nousr Yea, that seems to have fixed the issue. I don't have a good handle on why the split results in non-deterministic behavior though. Is it just the conversion between sets and lists that is messing up the parameter order?

Is it just the conversion between sets and lists that is messing up the parameter order?

@Veldrovive oh shoot! yeah....good catch--I didn't even see those set() calls up there in separate_weight_decayable_params(). I was so hyperfocused on get_optimizer() that once it worked with group_wd_params=False I was happy enough to continue on with my life😆
I bet changing those to list() would fix the issue, should be an easy test & PR (assuming it works) if you get some time.

The optimizer has been fixed and tested in #130 so it should no longer be necessary to use the group_wd_params=False option.

I brought the branch up with main and started a longer term test run with only the initial unet on 3 GPUs. You can track its progress here.

The training run above converged about as quickly as we would expect given its distributed batch size and all evaluation, saving, and logging works.

The change to model saving could be part of its own pull request for improving training features as it does not really fit under the umbrella of distributed training. I also want to change config logging to save the config dict as a file instead of in the wandb config object as that is more general so I could open a pull request for those features if we want to be good about keeping separate feature changes in separate requests. Up to whoever's merging if I should separate it out. I just needed the feature for testing in this branch.
I can also go and squash these commits if you do not do a squash during merge.

The training run above converged about as quickly as we would expect given its distributed batch size and all evaluation, saving, and logging works.

with accelerate, would we still have the option to go single GPU if need be? (given the big changes to the trainer class)

with accelerate, would we still have the option to go single GPU if need be? (given the big changes to the trainer class)

That's actually my favorite feature of accelerate. If you launch with python train_decoder.py you get bog standard training on the first available device and accelerate basically does nothing. You do have to abide by the rules of accelerate, however, which really only means it is harder to select which device to train on since you have to use the CUDA_VISIBLE_DEVICES environment variable.
The simplicity does come with the cost that in order to use extra features like mixed precision you have to use accelerate launch since accelerate handles scaling itself, but it works so well in every situation that it feels worth the slight inconvenience.

very cool and great job! i'll review it this weekend and we can try to have it merged on Monday :)

The training run above converged about as quickly as we would expect given its distributed batch size and all evaluation, saving, and logging works.

First of all, thanks very much about the contribution of your distributed training code. When running this decoder distributed training,   it will hang up in the line of self.accelerator.backward(loss) if make find_unused_parameters=True, it will hang up in the line of loss_map = {f"Unet {index} Training Loss": loss.item() for index, loss in enumerate(unet_average_loss)
if loss != 0}  (The output result of accelerator.gather(unet_losses_tensor) cannot be iterated) if make find_unused_parameters=False. Thanks again, looking forward to your reply.

First of all, thanks very much about the contribution of your distributed training code. When running this decoder distributed training, it will hang up in the line of self.accelerator.backward(loss) if make find_unused_parameters=True, it will hang up in the line of loss_map = {f"Unet {index} Training Loss": loss.item() for index, loss in enumerate(unet_average_loss) if loss != 0} (The output result of accelerator.gather(unet_losses_tensor) cannot be iterated) if make find_unused_parameters=False. Thanks again, looking forward to your reply.

That is very strange behavior. I assume you are editing the line ddp_kwargs = DistributedDataParallelKwargs(find_unused_parameters=True)?
Setting find_unused_parameters to False should error on the backward pass because unets.0.null_text_embed does not participate in the loss calculation since text encodings are not supported. Did you modify the code to use text encodings?
I don't know of anything that would cause it to hang on the dict comprehension. If it is hanging, it should be on a gather call. Can you print out the intermediate value between the gather and comprehension?

@1073521013 Ok, I don't have any idea what could be causing the issue. There are three options I see. It could be something I didn't account for that you are using in your config, a code change that doesn't seem like it should affect anything but still causes issues, or a torch/accelerate version mismatch. If you could post your config in a pastebin we can start eliminating options here.

@1073521013 Ok, I don't have any idea what could be causing the issue. There are three options I see. It could be something I didn't account for that you are using in your config, a code change that doesn't seem like it should affect anything but still causes issues, or a torch/accelerate version mismatch. If you could post your config in a pastebin we can start eliminating options there.

torch/accelerate version has matched(same problem). and my config:
{
"decoder": {
"unets": [
{
"dim": 512,
"image_embed_dim": 512,
"cond_dim": null,
"channels": 3,
"dim_mults": [1, 2, 3, 4],
"attn_dim_head": 64,
"attn_heads": 32
}
],
"image_sizes": [64],
"channels": 3,
"timesteps": 1000,
"loss_type": "l2",
"beta_schedule": "cosine",
"learned_variance": true
},
"data": {
"webdataset_base_url": "/home/notebook/data/group/ma/laion400M_data/laion400m-met-release/laion400m-webdataset/{}.tar",
"embeddings_url": "/home/notebook/data/group/laion_400_embeddings/laion400m-embeddings/img_emb",
"num_workers": 2,
"batch_size": 25,
"start_shard": 0,
"end_shard": 41512,
"shard_width": 5,
"index_width": 4,
"splits": {
"train": 0.9,
"val": 0.05,
"test": 0.05
},
"shuffle_train": true,
"resample_train": false,
"preprocessing": {
"RandomResizedCrop": {
"size": [64, 64],
"scale": [0.75, 1.0],
"ratio": [1.0, 1.0]
},
"ToTensor": true
}
},
"train": {
"epochs": 100,
"lr": 2e-5,
"wd": 0.1,
"max_grad_norm": 0.5,
"save_every_n_samples": 300000,
"n_sample_images": 6,
"device": "cuda:0",
"epoch_samples": 2000000,
"validation_samples": 50000,
"use_ema": false,
"ema_beta": 0.99,
"amp": false,
"save_all": false,
"save_latest": true,
"save_best": true,
"unet_training_mask": [true]
},
"evaluate": {
"n_evaluation_samples": 10,
"FID": {
"feature": 64
},
"IS": {
"feature": 64,
"splits": 10
},
"KID": {
"feature": 64,
"subset_size": 10
},
"LPIPS": {
"net_type": "vgg",
"reduction": "mean"
}
},
"tracker": {
"tracker_type": "wandb",
"data_path": "./models_all",
"image_path":"./wandb",
"wandb_entity": "foreveryoung",
"wandb_project": "decoder",
"verbose": false
},
"load": {
"source": null,
"run_path": "foreveryoung/decoder/azotfo37",
"file_path": "latest.pth",
"resume": false
}
}

Ah, ok the most likely explanation is that 400M has not been reordered so the image shards do not line up with the embeddings shards. If that happens one process will load a shard correctly and another won't which leads to a hang on the first gather it gets to. The reason it doesn't error is because webdataset hangs instead of erroring if an image shard it's trying to load doesn't exist.  This is also a problem if you use an end shard that is greater than the number of shards. This is a good thing to bring up because it will be one of the most common issues and it is worth addressing with better logging.
Logging won't solve the base issue of the dataset not working for training the decoder, though. Which source are you using to get 400M? I can probably reorder the embeddings so that it is a valid dataset.
We should also add an option to do on the fly embedding generation so that if a dataset does not have reordered embeddings and you don't have the ability to generate them yourself you can trade off speed and just load images.

This is indeed the case. My data is obtained from here . Due to some other problems, I downloaded the training data locally for training. I originally wanted to code to align it, but I saw that you have implemented embedding-dataset-reordering, so I used your script to align it. After the final alignment, I experimented with it. There are still two small problems, one is after alignment The embedding npy file has a large number of 0 matrices (which may represent images that were not successfully downloaded), resulting in lots of problems like UserWarning: RuntimeError('Webdataset had a sample, but no embedding was found. ImgShard: 10093 - Index: 0229'); Another problem is that although the distributed operation is running, a step only executes one batch instead of gpus*batch. It is still under research, thanks.

0 vectors are inserted into the embeddings whenever an image index was skipped when computing embeddings. That warning showing up in large quantities means a lot of image links were dead when embeddings were computed.

Another problem is that although the distributed operation is running, a step only executes one batch instead of gpus*batch.

I am not sure exactly what this means. Are you saying that sample only increases by batch size every step instead of by gpus*batch size?

This is indeed the case. My data is obtained from here . Due to some other problems, I downloaded the training data locally for training. I originally wanted to code to align it, but I saw that you have implemented embedding-dataset-reordering, so I used your script to align it. After the final alignment, I experimented with it. There are still two small problems, one is after alignment The embedding npy file has a large number of 0 matrices (which may represent images that were not successfully downloaded), resulting in lots of problems like UserWarning: RuntimeError('Webdataset had a sample, but no embedding was found. ImgShard: 10093 - Index: 0229'); Another problem is that although the distributed operation is running, a step only executes one batch instead of gpus*batch. It is still under research, thanks.

I carefully studied the data downloaded before, and found that the meta parquet data of embeddings/metadata/ is different from the parquet log data in webdataset that the download is successful, and they are not aligned, which leads to a large number of warnings;
The second problem is that my code is wrong. and it's solved; Many thanks for your hospitality.

Hi, thanks for your contribution. When I use your code, it casues an error shown in the following pic.

do you know how to fix it?

Hi, thanks for your contribution. When I use your code, it casues an error shown in the following pic.

do you know how to fix it?

Hey! This issue happens when you try to call the member function of a Distributed model.
The common solution for this is "unwrapping" the model by calling distributed_model.module.member_function().
Hopefully this puts you on the right track!

Hi, I am also confused. Where do I need to unwrapping the model?

Hi, thanks for your contribution. When I use your code, it casues an error shown in the following pic.

do you know how to fix it?

Hey! This issue happens when you try to call the member function of a Distributed model.
The common solution for this is "unwrapping" the model by calling distributed_model.module.member_function().
Hopefully this puts you on the right track!

Do you mean I need to fix it like this?

Hi, thanks for your contribution. When I use your code, it casues an error shown in the following pic.

do you know how to fix it?

Hey! This issue happens when you try to call the member function of a Distributed model.
The common solution for this is "unwrapping" the model by calling distributed_model.module.member_function().
Hopefully this puts you on the right track!

 Do you mean I need to fix it like this?

Thanks, I fix it!

Hi, I also have an issue. My dataset contains about 17million data. But during my training process, in epoch 0 it samples more than 17million. Why did it happen?

Hi, thanks for your contribution. When I use your code, it casues an error shown in the following pic.  do you know how to fix it?

That's on me. It is fixed in a commit I have not not pushed.

I also met a problem that yy dataset contains about 17million data, but during my training process, in epoch 0 it samples more than 17million. It couldn't move to epoch 1 and it failed t o save the checkpoint although I set the "save_all" as true in config.

Hi, sorry to bother you again. I met an issue after I loaded the checkpoints. I can load the model successfully. But when it starts training, it shows an error. I am confused about it. Why did this happen even if the model is loaded successfully?

Hey! This looks like a problem with the optimizer. I would ensure that you are utilizing the same hyper-parameters for the decoder when you saved the checkpoint. Additionally, I would make sure you have the latest version of the repo.
If you've tried both of those and still get the error, please open an issue with a minimal example to recreate the problem so we can try to get to the bottom of it!

Hi, sorry to bother you again. I met an issue after I loaded the checkpoints. I can load the model successfully. But when it starts training, it shows an error. I am confused about it. Why did this happen even if the model is loaded successfully?

Hey! This looks like a problem with the optimizer. I would ensure that you are utilizing the same hyper-parameters for the decoder as when you saved the checkpoint. Additionally, I would make sure you have the latest version of the repo (and it might be worth checking that this issue still presents itself if you save a checkpoint with the latest version as well).
If you've tried both of those and still get the error, please open an issue with a minimal example to recreate the problem so we can try to get to the bottom of it!

Thanks, I will try to download the latest version of the repo. Is that from https://github.com/Veldrovive/DALLE2-pytorch/tree/decoder_trainer_accelerate?

@Veldrovive hey Aidan! thanks for continuing to push on this pull request!
we can merge this any time you think it is ready
(don't worry about Imagen btw, i will probably complete it next week solo)

@lucidrains I brought it even with main to resolve merge conflicts yesterday so that it could be merged. Nousr and I just finished tracking down a bug that I thought might have been introduced, but it turned out it was just wandb having strange behavior. This is a run using the up to date version of the branch with sparse attention enabled for the unet. It is ready to be merged. Would you like me to squash the commits or jut leave it as is?

@Veldrovive it is perfect! let's merge it :pray:
@nousr @Veldrovive what is both of your emails btw? could you send me an email at<EMAIL_ADDRESS>so i can keep track?


Show PDF inputs fields appearing on work form

Some fields (show pdf viewer, show pdf download button) are showing on work creation pages. It is unclear whether text entered in these fields affects the work display page, but the information is saved in these fields, and the presence of the fields will be confusing to users.

Testing Instructions

EDIT a new work
In the work's form, click additional fields. Search for "Show PDF" and verify there are no matches.
[ ] There should be no input fields for show viewer or show pdf download button from this page.
Click on the Files tab.
[ ] if PDF.js is enabled (from the features settings of the admin dashboard), two checkboxes should appear to show viewer, and to show pdf download button.
[ ] If the PDF.js is NOT enabled, these checkboxes will not be visible

TODO: We need to figure out a way for get the yaml's form > display: false to actually hide the terms from the form. Right now it's getting ignored. If we completed remove the form block, then the backend won't have the term on the form and that causes other problems.

SoftServ QA: ❌
PDF.js not enabled
https://github.com/user-attachments/assets/7f579a89-318d-46e4-9377-e0317c15d9c6
pdf.js enabled ❌
The checkboxes are not apearing under the files tab
https://github.com/user-attachments/assets/7b92073d-7a24-4daa-9811-a4c0e860ff80

Hey @jillpe, can you try this with a PDF file?  It looks like you tried it with a JPEG which will not show in PDF.js because it's not a PDF.

SoftServ QA Observations
UV Enabled
https://github.com/user-attachments/assets/59ef11bb-3b14-4be5-9439-b6f8583631db
PDF.js enabled
The checkboxes are not appearing
https://github.com/user-attachments/assets/5ebf73cd-6a00-4ee1-bb71-80fac3d56a7f

Looking into this. locally every work type is displaying the checkboxes when pdf.js is enabled.

Staging is not displaying it. 🤔 Same sha: c663a10a

Local

VALKYRIE_TRANSITION wasn't set 🙃

QA RESULTS: PASS ✅
Fields do not show on Descriptions tab

Checkboxes show when PDF.js is enabled

Checkboxes do not show when PDF.js is disabled


Room is not disposed after all clients disconnect

The problem is that the room does not dispose even after all of the clients leave. Then if another player searches for room to join he gets connected to this room with the "ghost" client.
The room gets finally disposed only after the game is re-launched by the client who initially created the room. Weird.
This happens only if the clients leave by instantly closing the application.

void OnApplicationQuit()
{
    // Ensure the connection with server is closed immediatelly
    LeaveRoom();
    if (Colyseus != null) Colyseus.Close();
}

public void LeaveRoom()
{
    if (Room != null)
    {
        Room.Leave(true);
    }
}

void OnRoomLeft(object sender, EventArgs e)
{
    Room = null;
}

Fixed by reducing the function code to minimum:

void OnApplicationQuit()
{
// Ensure the connection with server is closed immediatelly
if (Colyseus != null) Colyseus.Close();
}


Alignment is incorrectly replaced by tabs

When using tabs as indentation and spaces as alignment, VS Code just replaces spaces with tabs when pressing return. The indentation and alignment should be preserved from the previous line.
There was already an issue which got 19! upvotes. That is one less than needed for the backlog. (#134618)
This is a major problem and should be adressed. In the current state, it slows down the coder when writing complex code.

This feature request is now a candidate for our backlog. The community has 60 days to upvote the issue. If it receives 20 upvotes we will move it to our backlog. If not, we will close it. To learn more about how we handle feature requests, please see our documentation.
Happy Coding!

Thank you for picking up the issue!
Here is a video showing the problem:
https://github.com/microsoft/vscode/assets/63237520/3f35fb19-44f1-4b22-94f4-07de7ba29fcd
VS Code currently uses as many tabs as it can, without changing the total width of whitespace in the line. But it would be good to preserve the spaces in this case. The intent is to use tabs for indentation and spaces for alignment.
Current behavior:
void func() {
[TAB]foobar(1,
[TAB]       2,  // spaces for alignment
[TAB][TAB]  3); // VS Code replaces the spaces with tabs after pressing enter
}

Desired behavior:
void func() {
[TAB]foobar(1,
[TAB]       2,  // spaces for alignment
[TAB]       3); // same on the next line after pressing enter
}

This feature request has not yet received the 20 community upvotes it takes to make to our backlog. 10 days to go. To learn more about how we handle feature requests, please see our documentation.
Happy Coding!

@hediet This is not really a feature request, as it makes the code style practically impossible to use. Could you please take a look at my comment? Thanks.

:slightly_frowning_face: In the last 60 days, this feature request has received less than 20 community upvotes and we closed it. Still a big Thank You to you for taking the time to create this issue! To learn more about how we handle feature requests, please see our documentation.
Happy Coding!

Related on Stack Overflow: Can I set VS Code to use tabs for indentation and spaces for leading alignment?
@alexdima how did #134618 get closed? And how is it simultaneously closed and on the backlog?

It's really disappointing to see both issues being handled as regular feature requests and getting closed. I answered on the same day the question was asked and didn't even get a reply. Seems that nobody at MS cares about this ):

@aiday-mar what do you think about this feature?

I think this is a valid feature request, and should be looked into at some point.


python: remove _manylinux.py

Motivation for this change
This will turn manylinux support back on by default.
PIP will now do runtime checks against the compatible glibc version to
determine if the current interpreter is compatible with a given
manylinux specification. However it will not check if any of the
required libraries are present.
Things done

[ ] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)

[ ] NixOS
[ ] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
[ ] Tested execution of all binary files (usually in ./result/bin/)
[ ] Determined the impact on package closure size (by running nix path-info -S before and after)
[ ] Ensured that relevant documentation is up to date
[ ] Fits CONTRIBUTING.md.

Notify maintainers
cc @FRidh

As discussed, the change is good, however, I would like the commit to describe a bit more the motivation (so what we discussed per chat).

As discussed, the change is good, however, I would like the commit to describe a bit more the motivation (so what we discussed per chat).

Yeah, will update that once we have that ;)

@Fridh let me know if the current commit message is good enough

Commit message looks good to me! Maybe also add an entry in the change log.


[WIP] Implement page 404 with react-router

@denisnikulin please continue by adding styles and i18n

Closing as stale

Closing as stale


ProperTorsionHandler should raise an error if ProperTorsionType has differing number of k, phase, and periodicity

Describe the bug
For a ProperTorsionType to make any sense, it needs to have the same number of kX, periodicityX, and phaseX values. Currently, outside of initialization, this is never checked. So, the way it's implemented currently, a user could append a new kX value, but not a corresponding phaseX or periodicityX, and the OFF Toolkit will just silently ignore any unpaired values.
To Reproduce
from openforcefield.topology import Molecule
from openforcefield.typing.engines.smirnoff import ForceField
from simtk import unit
ethanol = Molecule.from_smiles('CCO')
ff = ForceField('openff-1.0.0.offxml')
param = ff.get_parameter_handler('ProperTorsions').parameters['[#6X4:1]-[#6X4:2]-[#8X2H1:3]-[#1:4]']
param.k.append(1000.*unit.kilocalorie_per_mole)
print(ff.get_parameter_handler('ProperTorsions').parameters['[#6X4:1]-[#6X4:2]-[#8X2H1:3]-[#1:4]'])
sys = ff.create_openmm_system(ethanol.to_topology())

<ProperTorsionType with smirks: [#6X4:1]-[#6X4:2]-[#8X2H1:3]-[#1:4]  periodicity1: 3  periodicity2: 1  phase1: 0.0 deg  phase2: 0.0 deg  k1: 0.6200339965628 kcal/mol  k2: 0.1559715289248 kcal/mol  k3: 1.0 kcal/mol  id: t85  idivf1: 1.0  idivf2: 1.0  >

So we DO indeed modify the parameter in the ForceField.
The new parameter is not in the resulting system
from simtk.openmm import XmlSerializer
print(XmlSerializer.serialize(sys))

...
		<Force forceGroup="0" type="PeriodicTorsionForce" usesPeriodic="0" version="2">
			<Torsions>
				<Torsion k="2.5942222416187555" p1="0" p2="1" p3="2" p4="8" periodicity="3" phase="0"/>
				<Torsion k=".6525848770213633" p1="0" p2="1" p3="2" p4="8" periodicity="1" phase="0"/>
				<Torsion k=".7356182516976472" p1="2" p2="1" p3="0" p4="3" periodicity="3" phase="0"/>
				<Torsion k="-2.020274253385594" p1="2" p2="1" p3="0" p4="3" periodicity="1" phase="0"/>
				<Torsion k=".7356182516976472" p1="2" p2="1" p3="0" p4="4" periodicity="3" phase="0"/>
				<Torsion k="-2.020274253385594" p1="2" p2="1" p3="0" p4="4" periodicity="1" phase="0"/>
				<Torsion k=".7356182516976472" p1="2" p2="1" p3="0" p4="5" periodicity="3" phase="0"/>
				<Torsion k="-2.020274253385594" p1="2" p2="1" p3="0" p4="5" periodicity="1" phase="0"/>
				<Torsion k=".4371885647050472" p1="3" p2="0" p3="1" p4="6" periodicity="3" phase="0"/>
				<Torsion k=".4371885647050472" p1="3" p2="0" p3="1" p4="7" periodicity="3" phase="0"/>
				<Torsion k=".4371885647050472" p1="4" p2="0" p3="1" p4="6" periodicity="3" phase="0"/>
				<Torsion k=".4371885647050472" p1="4" p2="0" p3="1" p4="7" periodicity="3" phase="0"/>
				<Torsion k=".4371885647050472" p1="5" p2="0" p3="1" p4="6" periodicity="3" phase="0"/>
				<Torsion k=".4371885647050472" p1="5" p2="0" p3="1" p4="7" periodicity="3" phase="0"/>
				<Torsion k=".9452409620490081" p1="6" p2="1" p3="2" p4="8" periodicity="3" phase="0"/>
				<Torsion k=".9452409620490081" p1="7" p2="1" p3="2" p4="8" periodicity="3" phase="0"/>
			</Torsions>
		</Force>
...

May try rolling this in to the addition on #572, since we need similar checks for kX_bondorderYs.

Gotcha. If this fix comes easily, great. But if it gets at all complicated, feel free to save this issue for a later PR. I've found that, once I start doing little +1s on my PRs, the list gets very long very fast.

Where are you thinking the error should be raised? I assume we'd want to allow adding the values one-by-one (i.e. not erroring out after adding k3 simply because periodicity3 isn't defined yet). I don't know enough about how the XMLs are validating while being loaded in (is there a validation function that can be called later?). It looks like orphaned parameters are just never matched to, causing their silent death. So creating the forces probably isn't the place to do this. A validation check early in ForceField.create_openmm_system would fix this for this case, but it may also be nice to have a separate function that chops off stuff like this, warning or erroring out along the way.

I'd vote for checking in ProperTorsionHandler.create_force, before we do the parameter application. So basically, we'd check for list length equality right before that zip loop.
Now that we understand this use case (and the complications with e.g. partial bond orders), this is something that we could build into the design of ParameterType when we get around to doing a refactor.

I think the best place to put these validation checks is here.

I like the idea of fixing this behavior in a more specific place now and thinking about a more general feature later


Some fixes/changes to the documentation

Fixes #86, #85, #77, and #84

All the changes to the qmd/md files involves the angle brackets issue
Had to change the h1 font-size in Shane's CSS to 40px so that the logo text did not take up 3 lines

can we remove the authors section, or just use OHRI as the author?

Yes, we can use OHRI as the author<EMAIL_ADDRESS>@yulric, you can continue to be an author if you want.. I think it makes sense to have people's names, but I am totally fine if @zargot doesn't want to be named.

@zargot I believe I've removed your name from the commits and the file. Let me know if it all looks good.


[Fix #6748] Fix Style/RaiseArgs auto-correction breaking in contexts that require parentheses

This cop would produce invalid code, such as:
foo ? bar : raise FooError, 'baz'

in contexts where the call to raise requires parentheses. This change fixes that by adding parentheses, making the correction:
foo ? bar : raise(FooError, 'baz')

This treatment is applied in ternaries and when used in logical operators.

Before submitting the PR make sure the following are checked:

[X] Wrote good commit messages.
[X] Commit message starts with [Fix #issue-number] (if the related issue exists).
[X] Feature branch is up-to-date with master (if not - rebase it).
[X] Squashed related commits together.
[X] Added tests.
[X] Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
[X] The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
[X] Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

Thanks!


Recommended workflow for forked PR

Danger can't update the status on a forked PR and when it's a required check, we can't merge the PR as a result.

"::warning::Running from a forked repo. Danger won't be able to post comments and workflow status on the main repo, printing directly."

What's the recommended way to configure this stuff? Some docs/explanation on what to do would be nice.

TLDR; don't make the workflow a required check - it will still run and whoever is merging can check the its green before hitting the merge button. I know it's not great...
There may be a workaround but so far it was deemed too much trouble to try and get it working (and it also may need to be done for each repo separately so out of scope here).
See the same discussions in this issue https://github.com/getsentry/sentry-dotnet/issues/1004


Add YouTube client for 1080p muxed videos

Description of your pull request and other information

Adds the ability for yt-dlp to return premuxed 1080p results for videos when available.

Template

Before submitting a pull request make sure you have:

[x] At least skimmed through contributing guidelines including yt-dlp coding conventions
[x] Searched the bugtracker for similar pull requests
[x] Checked the code with flake8 and ran relevant tests

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

[x] I am the original author of this code and I am willing to release it under Unlicense
[ ] I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

[x] Fix or improvement to an extractor (Make sure to add/update tests)
[ ] New extractor (Piracy websites will not be accepted)
[ ] Core bug fix/improvement
[ ] New feature (It is strongly recommended to open an issue first)

I just tested this out, and this started happening to me on the 3rd video:
WARNING: [youtube] Skipping player responses from android clients (got player responses for video "aQvGIIdgFDM" instead of "BaW_jenozKc")

Definitely should not be in the default clients

Before I got blocked by youtube (?) I was going to write that I think adding another default request to manifest.googlevideo.com is probably unwise and will get people rate-limited

I'm not too concerned about it being on by default or not, i just added it there while testing with vrchat to check the returned streams were compatible with avpro easier, as long as it can be enabled with a command line option I'm fine with it not being on defaults.

I just tested this out, and this started happening to me on the 3rd video:
WARNING: [youtube] Skipping player responses from android clients (got player responses for video "aQvGIIdgFDM" instead of "BaW_jenozKc")

Note that while it says "android clients", it was for the media_connect_frontend client API request.
Maybe unrelated, but this has never happened to me before. IMO should not be in the default clients

In the last 12 hours I have seen that on two different videos, but there are other videos that don't produce this warning.  aQvGIIdgFDM was claimed to be returned in both instances.  I am using 2<IP_ADDRESS>247.  This should probably be tracked as a separate issue.

@rredford6 I only get the bad response with the new client. It pertains to this PR

So then how am I able to reproduce with 2<IP_ADDRESS>247?

@rredford6 perhaps you should open a new issue then

"Wrong player response" issue has been confirmed to not be due to this PR and should be tracked separately

You can get the 1080p muxed videos by using a safari user agent with the web client - no need for another client.
I'm guessing media_connect_frontend is android - the android clients are starting to require droidguard, making them unusable for yt-dlp. We will probably be removing them in due course.

You can also get the 1080p muxed videos by using a safari user agent with the web client, so could also add a duplicate of web client with the safari user agent.
~I'm guessing media_connect_frontend is android - the android clients are starting to require droidguard, making them unusable for yt-dlp. We will probably be removing them soon.~ nvm, it seems fine. Worth adding anyways as a backup.

 I can't seem to reproduce this, Am I doing something wrong here?
yt-dlp  https://www.youtube.com/watch?v=i1csLh-0L9E --list-formats --user-agent "Mozilla/5.0 (Macintosh; Intel Mac OS X 14_4_1) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.3.1 Safari/605.1.15" --extractor-args "youtube:player_client=web"

try --impersonate safari

try --impersonate safari instead of passing UA

Yep, works with that, thanks

Is there a way to only impersonate safari for a specific extractor?

there will be when #9474 is merged

This yt-dlp -gf 96 tIIWYh601tg --impersonate safari gives me .m3u8 playlist and not the direct mp4 link. What am I missing here?

@Elite safari is served m3u8 formats. don't pass --impersonate safari

Removing that also results in same!
2024-06-07_09-06

@Elite the mediaconnect client is also served m3u8 formats. Why are you using it if you want http mp4 formats? and you are passing -f 96 which will always select an m3u8 format

@Elite I think you are conflating being pre-merged and the protocol used. Format 96 is pre-merged (audio and video are together), but it is transmitted with segments in a m3u8 and not as a single mp4 file. Afaik. there is no pre-merged http mp4 URL.

@seproDev Yes, that was the expectation getting premuxed mp4 over http, thanks for clarifying.

One question though, is this format 96 hls stream downloadable only from the IP it was generated or can it be used from other IP as well?

@Elite depends on the video. The original requestor's IP address is always embedded in the URL, but the regional/IP restriction is not always enforced


add ability to retrieve ROI names in a case insensitive way

HI,
i'm experiencing problem with low quality ROI labels (written like this: Heart, heart, HEART),
maybe an option to extract case insensitive ROIs could be useful to other also
I have a quick_fix in rtstruct.py:98
if structure_roi.ROIName.casefold() == name.casefold():
maybe a more general option while using ".get_roi__mask_by_name" could be better

Hi @raffaelespinoni, I'm thinking we add an option in get roi_mask_by_name to enable case insensitivity using the code you've provided. I'll try to get this done for this wednesday but feel free to submit your own pull request if you'd like :)

Thank's, i'm opening a PR