tf.nn.separable_conv2d is slower than conv2d on GPU

System information

Have I written custom code (as opposed to using a stock example script provided in TensorFlow):Yes
OS Platform and Distribution (e.g., Linux Ubuntu 16.04): Ubuntu 16.04
TensorFlow installed from (source or binary): binary
TensorFlow version (use command below): TF 1.3
Python version: 3.6
Bazel version (if compiling from source):
CUDA/cuDNN version: CUDA8.0 /cuDNN6
GPU model and memory: GTX1080ti  11G

Describe the problem
In theory, separable_conv2d should be more efficient than conv2d, but when I test a simple model on Cifar10, the result shows that nn.separable_conv2d run slower on GPU, but is indeed faster on CPU.
Here is my test results on GPU:
training time for normal_conv after 2000 step: 8.18395892999979 sec
time for normal_conv after one forward step:  0.003980965999289765 sec
training time for separable_conv after 2000 step: 9.158266903999902 sec
time for separable_conv after one forward step:  0.0036441169995669043 sec

Source code / logs
Below is a fully self-contained example, I first define a model with two  conv2d , than I define another model with one conv2d followed by one  separable_conv2d, both model have 32 channels for each conv_layer and identical fc_layer.
import tensorflow as tf
import timeit
import numpy as np
from tensorflow.contrib.keras.python.keras.datasets.cifar10 import load_data

(x_train, y_train), (x_val, y_val) = load_data()
learning_rate = 0.001
num_steps = 1000
n_classes = 10
batch_size = 32

def reformat(labels):
    # Map 0 to [1.0, 0.0, 0.0 ...], 1 to [0.0, 1.0, 0.0 ...]
    labels = (np.arange(n_classes) == labels[:,None]).astype(np.float32)
    return  labels.reshape(labels.shape[0],10)
train_labels = reformat(y_train)
tf.reset_default_graph()
x = tf.placeholder(tf.float32, [None, 32, 32, 3])
y = tf.placeholder(tf.float32, [None, 10])
weights1 = {}
weights2 = {}
dtype = tf.float32
with tf.name_scope('INIT_OP'):
    conv_initializer =  tf.contrib.layers.xavier_initializer_conv2d(dtype=dtype)
    fc_initializer =  tf.contrib.layers.xavier_initializer(dtype=dtype)

k = 3
kernel = 16

# Define weights for normal ConvNet
with tf.name_scope('VARIABLES_1'):
    weights1['conv1'] = tf.get_variable('conv1', [k, k, 3, kernel], initializer=conv_initializer, dtype=dtype, trainable=True)
    weights1['b1'] = tf.get_variable('b1', initializer=tf.zeros([kernel]))
    weights1['conv2'] = tf.get_variable('conv2', [k, k, kernel, kernel], initializer=conv_initializer, dtype=dtype, trainable=True)
    weights1['b2'] = tf.get_variable('b2', initializer=tf.zeros([kernel]))

    weights1['wd1'] = tf.get_variable('wd1', [8*8*kernel, 512], initializer=fc_initializer, dtype=dtype, trainable=True)
    weights1['bd1'] = tf.get_variable('bd1',  initializer=tf.zeros([512]) )
    weights1['wd2'] = tf.get_variable('wd2', [512, 10], initializer=fc_initializer, dtype=dtype, trainable=True)
    weights1['bd2'] = tf.get_variable('bd2',  initializer=tf.zeros([10]) )

#Define weights for separable ConvNet
with tf.name_scope('VARIABLES_sep'):
    weights2['conv1'] = tf.get_variable('2_conv1', [k, k, 3, kernel], initializer=conv_initializer, dtype=dtype, trainable=True)
    weights2['conv_dw2'] = tf.get_variable('conv_dw2', [k, k, kernel, 1], initializer=conv_initializer, dtype=dtype, trainable=True)
    weights2['conv_pw2'] = tf.get_variable('conv_pw2', [1, 1, kernel, kernel], initializer=conv_initializer, dtype=dtype, trainable=True)

    weights2['b1'] = tf.get_variable('2_b1', initializer=tf.zeros([kernel]))
    weights2['b2'] = tf.get_variable('2_b2', initializer=tf.zeros([kernel]))

    weights2['wd1'] = tf.get_variable('2_wd1', [8*8*kernel, 512], initializer=fc_initializer, dtype=dtype, trainable=True)
    weights2['bd1'] = tf.get_variable('2_bd1',  initializer=tf.zeros([512]) )
    weights2['wd2'] = tf.get_variable('2_wd2', [512, 10], initializer=fc_initializer, dtype=dtype, trainable=True)
    weights2['bd2'] = tf.get_variable('2_bd2',  initializer=tf.zeros([10]) )

def forward_conv_sep( inp, weights):
    hidden = conv_block(inp, weights2['conv1'], weights2['b1'])
    hidden = maxpool2d(hidden)
    hidden = conv_block_dw(hidden, weights2['conv_dw2'], weights2['conv_pw2'], weights2['b2'])
    hidden = maxpool2d(hidden)
    hidden = tf.reshape( hidden, [-1, np.prod([int(dim) for dim in hidden.get_shape()[1:]])] )
    fc1 = tf.matmul(hidden, weights2['wd1']) + weights2['bd1']
    fc1 = tf.nn.relu(fc1)
    return tf.matmul(fc1, weights2['wd2']) + weights2['bd2']

def forward_conv( inp, weights):
    hidden = conv_block(inp, weights1['conv1'], weights1['b1'])
    hidden = maxpool2d(hidden)
    hidden = conv_block(hidden, weights1['conv2'], weights1['b2'])
    hidden = maxpool2d(hidden)
    hidden = tf.reshape( hidden, [-1, np.prod([int(dim) for dim in hidden.get_shape()[1:]])] )
    fc1 = tf.matmul(hidden, weights1['wd1']) + weights1['bd1']
    fc1 = tf.nn.relu(fc1)
    return tf.matmul(fc1, weights1['wd2']) + weights1['bd2']

def conv_block_dw(inp, cweight_w, cweight_p, bweight):
    no_stride =  [1,1,1,1]
    conv_output = tf.nn.separable_conv2d(inp, cweight_w, cweight_p, no_stride, 'SAME') + bweight
    return tf.nn.relu(conv_output)

def conv_block(inp, cweight, bweight, activation=tf.nn.relu):
    no_stride =  [1,1,1,1]
    conv_output = tf.nn.conv2d(inp, cweight, no_stride, 'SAME') + bweight
    return tf.nn.relu(conv_output)

def maxpool2d(inp, k=2):
    return tf.nn.max_pool(inp, ksize=[1, k, k, 1], strides=[1, k, k, 1],
                          padding='SAME')

#logits for normal ConvNet
with tf.name_scope("forward_conv"):
    pred1 = forward_conv(x, weights1)

#Cost for normal ConvNet
with tf.name_scope("cost1"):
    loss1 = tf.nn.softmax_cross_entropy_with_logits(logits=pred1, labels=y)
    cost1 = tf.reduce_mean(loss1)

#training op for normal ConvNet
with tf.name_scope('train_op1'):
    train_op1 = tf.train.RMSPropOptimizer(learning_rate, 0.9).minimize(cost1)

#logits for separable ConvNet
with tf.name_scope("forward_conv_sep"):
    pred2 = forward_conv_sep(x, weights2)

#Cost for separable ConvNet
with tf.name_scope("cost2"):
    loss2 = tf.nn.softmax_cross_entropy_with_logits(logits=pred2, labels=y)
    cost2 = tf.reduce_mean(loss2)

# training op for separable ConvNet
with tf.name_scope('train_op2'):
    train_op2 = tf.train.RMSPropOptimizer(learning_rate, 0.9).minimize(cost2)

with tf.name_scope('INIT'):
    init = tf.global_variables_initializer()

with tf.Session() as sess:

    sess.run(init)

    #train normal ConvNet for 2000 steps
    start = timeit.default_timer()
    for step in range(num_steps):
        r = np.random.choice(y_train.shape[0], batch_size, replace=False)
        batch_data = x_train[r]
        batch_labels = train_labels[r]

        feed_dict = {x : batch_data, y: batch_labels}
        _ , l = sess.run([train_op1,cost1], feed_dict=feed_dict)

    stop = timeit.default_timer()
    print ('training time for normal_conv after '+str(num_steps)+' step:',stop - start)

    start = timeit.default_timer()
    feed_dict = {x : batch_data, y: batch_labels}
    predictions1 = sess.run(pred1, feed_dict=feed_dict)
    stop = timeit.default_timer()
    print ('time for normal_conv after one forward step: ',stop - start)

    # train separable ConvNet for 2000 steps
    start = timeit.default_timer()
    for step in range(num_steps):
        r = np.random.choice(y_train.shape[0], batch_size, replace=False)
        batch_data = x_train[r]
        batch_labels = train_labels[r]

        feed_dict = {x : batch_data, y: batch_labels}
        _ , l = sess.run([train_op2,cost2], feed_dict=feed_dict)

    stop = timeit.default_timer()
    print ('training time for sep_conv after '+str(num_steps)+' step:',stop - start)

    start = timeit.default_timer()
    feed_dict = {x : batch_data, y: batch_labels}
    predictions = sess.run(pred2, feed_dict=feed_dict)
    stop = timeit.default_timer()
    print ('time for sep_conv after one forward step: ',stop - start)

@vrv my recollection is that there are some issues with backprop for separable convolutions on GPUs, which had been somewhat improved lately. Can you comment please on the state of the art?

I'm not aware of any specific issues, kernels get faster as important models need them!  In general, convolutions of different sizes will have different performance characteristics, and it's possible our separable convolution implementations may be slow for some combinations of shapes.  I'm not sure whether that's the case here, but it could be.  I also don't know whether theory matches practice here, since separable convolutions are less compute dense than normal convolutions.  I believe the benefits you get are that you get to use fewer parameters to express a larger capacity convolution.

At what kernel sizes will convolutions be computed via FFT instead of directly? Anyway, a speedup by doing a separable convolution is more noticeable for larger kernels, so for small kernels the overhead involved in doing two convolutions will be larger than the speedup, especially for what I assume is a highly-optimized convolution setting with the 3x3 kernel (Winograd).
Essentially, for a [m, n] kernel it would take m*n calculations for a convolution and m+n calculations for the separable convolution, if I'm not mistaken.

@carlthome @vrv  I do notice that when the number of filters get larger (128 or 192 etc. ), separable_conv2d is faster than conv2d, but in my case, I applied separable_conv2d on cifar10 with small number of filters, and it is actually slower than conv2d on my GPU, what could be the cause?

Same principle. For small kernels matrices the overhead involved in doing two convolutions might be larger than the speedup.

Understood, thanks for the explanation!

It has been 14 days with no activity and the awaiting tensorflower label was assigned. Please update the label and/or status accordingly.

Why is this closed?  I implemented UNet with separable conv2d and it was around 80% slower than using a standard conv2d.  Is there anything in the works to optimize this like using groups?

Same problem. @HouseOfFinwe Did you figure out to fix this?

@keunwoochoi I did not, please let me know if either of you managed.

@HouseOfFinwe Ok thanks. https://github.com/tensorlayer/tensorlayer/issues/416#issuecomment-374180396 says the speed issue was fixed in tf 1.5, but I'm having it with 1.13 now. Do you remember which versions have you tried?

Sadly I do not

@HouseOfFinwe Ok thanks. tensorlayer/tensorlayer#416 (comment) says the speed issue was fixed in tf 1.5, but I'm having it with 1.13 now. Do you remember which versions have you tried?

Still exited on 1.14.

why close ??? still happen with tf1.13

This should be fixed by #33836. It currently requires tf-nightly but will ship in the coming 2.2 release.


Issue with jumping

The issue is when I'm on my server, and I perform /jump

I am teleported into a position like this:

to the side of the block. I think this may be an issue with Sponge teleporting the player to the intersection of four blocks, rather than in the middle of one block. See here for how I had to deal with that in my own plugin. I've experimented with it and it does different things in different directions.
As I see it, there are a couple ways to handle it:

Teleport the player to the front of the block, not through it.
Teleport the player on top of the block (probably a bad idea)
Teleport the player past the block (probably more similar to old essentials).

Thoughts?
Using EssentialCmds 7.5b
Using SpongeForge 1151

An update:
The ray seems to prefer east of all directions. It may be an issue with SpongeCommon (not sure really). I did some experimenting around and can't seem to find the root of the issue, so if you could find it or figure if it has something to do with SpongeCommon, that'd be great.
Also, teleporting a player through the block is also probably a bad idea (recently noticed /thru does this :P).


Better Engima

We should be using our version of Engima. We can host it on maven.concern.i.ng until we get an official maven server.

Why not just PR our changes upstream?

We should probably do that.

https://github.com/QuiltMC/enigma/pull/33

@SylvKT  @sschr15 all prs from the duvet team have been merged upstream!


ARROW-2061: [C++] Make tests a bit faster with Valgrind

Saves around 80 seconds on Travis-CI.

Codecov Report

Merging #2377 into master will increase coverage by 2.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2377      +/-   ##
==========================================
+ Coverage   84.51%   86.85%   +2.33%
==========================================
  Files         293      237      -56
  Lines       45329    42564    -2765
==========================================
- Hits        38312    36968    -1344
+ Misses       6990     5596    -1394
+ Partials       27        0      -27

Impacted Files
Coverage Δ

cpp/src/arrow/ipc/ipc-read-write-test.cc
99.04% <ø> (-0.01%)
:arrow_down:

cpp/src/arrow/compute/compute-test.cc
99.49% <100%> (-0.01%)
:arrow_down:

cpp/src/arrow/array-test.cc
100% <100%> (ø)
:arrow_up:

cpp/src/arrow/io/io-memory-test.cc
100% <100%> (ø)
:arrow_up:

cpp/src/arrow/util/thread-pool-test.cc
98.91% <0%> (-0.55%)
:arrow_down:

go/arrow/array/numericbuilder.gen.go

go/arrow/math/int64_sse4_amd64.go

go/arrow/math/int64_avx2_amd64.go

go/arrow/memory/checked_allocator.go

... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 992b27f...43a1e0e. Read the comment docs.


Add /api/recommendations

 Make a page in COMPSTAT with all DOIs of recommendations + DOIs or URLs of corresponding recommended preprints


docs: fix typo, tipes -> tips

thanks!

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

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot :package::rocket:


chrome.commands integration

Is it possible to integrate the chrome.commands API (https://developer.chrome.com/extensions/commands) so we can define shortcuts maybe for just opening the CKP popup for a password search etc.

I would accept a pull request that did this, but I am unlikely to work on it myself.
My preference for this need is to release a CKP App (different than the extension) where you would have a more KeePass-like experience for looking up and adding/editing passwords.  That is a lot of work though, so it won't happen soon.

Ok i will check this.


Visual P300 event markers not recorded

ℹ Computer information

Platform OS: Windows 10
Python Version: Python 3.7.10
Brain Interface Used: Muse 2

📝 Provide detailed reproduction steps (if any)

switch on your Muse device
start streaming using BlueMuse or Petal Metrics (I tried with both and had the same issues)
run the experiment with the command eegnb runexp -ip, and input the necessary parameters
complete the experiment

✔ Expected result
The title of the last column of the output/experimental data file is "Marker" and below it starting from the next row are zero markers denoting that no picture is presented at this time
❌ Actual result
The title of the last column is missing, and zeros start from data row 13 (row 14 of the file)
📷 Screenshots (this experimental was done using BlueMuse for streaming)

I think this is a duplicate of #108

The column is missing values initially: starting from data row 13 (or row 14 of the file) until the end of the file the values are present. I was wondering whether I can trust that the values that are present are aligned with the correct timestamp, or the fact that the markers start from row 13/14 means that they are shifted in time - I mean the marker on row 14 should really be on row 2?

I'm fairly confident that they are aligned, but @JohnGriffiths might have a more authoritative answer.

I agree. The lsl process writes to file what is received on the the current stream at each time point. I'm not aware of any way it would buffer or otherwise delay the stream output. Especially from the markers stream, which is running locally (as opposed to the EEG stream, which operates on Bluetooth data packets that are known to have a minimum and a variable lag).

Hi, I can confirm this issue. I was able to track it down - in part - to the muselsl-record function, where there is an if-condition that has a "save every 5s"-comment. Messing with this "catches" more markers (although it doesn't yet cure my problem).  Hope this helps. I will keep digging myself into it a bit more.

Closing to concatenate in #108.


[BUG] Intermediate dropout and weight decay settings seem to have no effect

🐛 Bug
Intermediate dropout and weight decay settings seem to have no impact on models (at least for CodeLlama models)
To Reproduce
I've tested almost all available hyperparameters for non-rlhf training, and all have observable impacts on the model (as evidenced by perplexity and other measures of downstream performance), except for intermediate dropout and weight decay. In the debugger, I can see intermediate dropout setting hidden_dropout_prob and attention_probs_dropout_prop in update_backbone_config, and I can see weight_decay being set in get_optimizer, however regardless of whether I set intermediate dropout to 0 or 0.5, and weight decay at 0 or 0.5, there is no change in the perplexity after model training.
LLM Studio version
Master latest commit

I should note that I'm doing a full fine tune and not using LORA.

Thanks @maxjeblick, that makes more sense now. Is there any fundamental reason why the Llama model config doesn't expose dropout, or was this just an oversight that could be remedied? (Found this issue but there has been no clear answer given yet)
Regarding weight_decay, yes I don't see any changes at all in the perplexity when changing it anywhere from 0 to very high values (i.e. 0.5), but easily see changes with even small changes of parameters like learning rate or gradient clip. Were you able to see changes in model validation metrics when changing weight_decay?

I'll investigate the weight decay peculiarity these days.
W.r.t. dropout, I'm not sure what is the reason behind disabling this feature completely. I assume that llama was (pre-) trained without dropout, and the authors didn't bother adding it back.

Sorry for the late reply!
I've checked training of CodeLlama-7b-Python-hf with default settings, once with weight decay 0, once with weight decay 0.5, and validation metrics are different.

Regarding the dropout, I'll create a PR that will handle this better.


Use go-bindata for assets

https://github.com/jteeuwen/go-bindata

Note: With this change, chef-runner can no longer be installed via go get.


Cannot find Subscription with name xxxxxxxx-xxxx-xxxxxxxxxxxx

Repro steps:

Clicked on "Deploy to webApp"
Selected the folder of my webApp
Selected my Free Plan (just created so empty)
And then seeing the error occured and crying

Usefull information : I've got an error in my Azure App Extension ! Under "Free Trial" there is "Error : Cannot find Subscription with name xxxxxxxx-xxxx-xxxxxxxxxxxx." but I saw on the internet that it was because ain't got anything in my account and it wasn't important...
Action: appService.Deploy
Error type: object
Error Message: {"key":"1","value":{"_creator":null,"_nextState":null,"_state":{"name":"error","done":null,"then":null},"_value":{"statusCode":404,"request":{"rawResponse":false,"queryString":{},"url":"https://management.azure.com/subscriptions/ xxxxxxxx-xxxx-xxxxxxxxxxxx/providers/Microsoft.Web/sites?api-version=2016-08-01","method":"GET","headers":{"x-ms-client-request-id":"xxxxxxxx-xxxx-xxxxxxxxxxxx","accept-language":"en-US","Content-Type":"application/json; charset=utf-8","user-agent":"Node/v7.9.0 (x64-Linux-4.4.0-121-generic) ms-rest/2.3.3 ms-rest-azure/2.5.5 azure-arm-website/1.0.0-preview Azure-SDK-For-Node"},"body":null},"response":{"body":"{"Code":"NotFound","Message":"Cannot find Subscription with name  xxxxxxxx-xxxx-xxxxxxxxxxxx.","Target":null,"Details":[{"Message":"Cannot find Subscription with name xxxxxxxx-xxxx-xxxxxxxxxxxx."},{"Code":"NotFound"},{"ErrorEntity":{"ExtendedCode":"51004","MessageTemplate":"Cannot find {0} with name {1}.","Parameters":["Subscription"," xxxxxxxx-xxxx-xxxxxxxxxxxx"],"Code":"NotFound","Message":"Cannot find Subscription with name  xxxxxxxx-xxxx-xxxxxxxxxxxx."}}],"Innererror":null}","headers":{"cache-control":"no-cache","pragma":"no-cache","content-length":"519","content-type":"application/json; charset=utf-8","expires":"-1","strict-transport-security":"max-age=31536000; includeSubDomains","x-ms-ratelimit-remaining-subscription-reads":"14997","server":"Microsoft-IIS/10.0","x-aspnet-version":"4.0.30319","x-powered-by":"ASP.NET","x-ms-request-id":"ad980a2a-733d-4e1e-a745-014d5da30cab","x-ms-correlation-request-id":"ad980a2a-733d-4e1e-a745-014d5da30cab","x-ms-routing-request-id":"FRANCECENTRAL:20180424T160207Z:ad980a2a-733d-4e1e-a745-014d5da30cab","x-content-type-options":"nosniff","date":"Tue, 24 Apr 2018 16:02:06 GMT","connection":"close"},"statusCode":404},"body":{}},"_listeners":null,"_isException":false,"_errorId":7}}
Version: 0.7.1
OS: Ubuntu 16.04

Are you behind a proxy? If so, you may need to set HTTP_PROXY and HTTPS_PROXY environment variables to get things working correctly.
Something like
export HTTPS_PROXY=http://username:password@proxy:8080
export HTTP_PROXY=http://username:password@proxy:8080

No but I finally find out a solution in creating my first webapp from the azure website and then using vs code extension for other webapp ! Maybe something badly initialized somewhere ... but now it's working fine for me ! =)

Not sure how/if we can fix the main issue, but we can at least show a better error message than all that json

Re-opening this as it consistently reproduces for new subscriptions. Turns out we have to make sure the 'Microsoft.Web' provider is registered


Mechanism for providing 6 character codes to people in order for them to join a program

Next: the mechanism for providing 6 character codes to people in order for them to join a program.

Originally posted by @simonw in https://github.com/natbat/pillarpointstewards/issues/72#issuecomment-1751437164

I'm going to generate 6 character codes using letters that are not likely to be confused with each other and avoiding vowels to avoid accidental swear words.
''.join(random.choice('BCDFGHJKMNPQRSTVWXYZ') for i in range(6))


Regarding @rimbu/deep match() latest version

Environment

Reproduction
Arrays and Tuples
// compound matches
const result = match(
  [
    { x: 1, y: 2 },
    { x: 3, y: 0 },
  ],
  { some: [{ x: (v) => v > 5 }, { y: (v) => v < 2 }] }
)

console.log(result) // false

The example suggests that the return value should be true
Function Matchers
match(
  {
    a: 1,
    b: { c: 2, d: { e: 3 } },
  },
  {
    b: {
      d: { e: (value, parent, root) => value === parent.c + root.a },
    },
  }
); // => true

// instead of returning the boolean in the previous statement, it is also possible to directly match the value
match(
  {
    a: 1,
    b: { c: 2, d: { e: 3 } },
  },
  {
    b: {
      d: { e: (value, parent, root) => parent.c + root.a },
    },
  }
); // => true

Both matchers actually return false for me.

Indeed, something seems to be off there, thanks for submitting. I remember reading about a plugin that can run documentation examples, I should look into that.
I'll investigate the cause.

The documentation is excellent as is - just being diligent before I commit using the lib in my project.

Rightfully so. The examples were incorrect, I updated them so that they should work. I also took the opportunity to add more array matching features, including array traversal matchers (see the new docs). I should Probably freeze the API for now, it's now possible to write crazy complex and powerful matchers. Not that anyone necessarily should do that though.


docs: fix Makefile for yaml docs generation

This commit is already in master, 17.05.x, 17.03.x, but somehow missed 17.04.x. Without it, you can't actually generate the YAML files correctly.

LGTM


feat(TokenCache): provide resolver factory

Add factory function on tokenCache to have an
easy way to obtain an access_token
See #17

@awaltert can you use 2 space indents and fix indentation? Other than that its nice :)

@bzums alright, sorry for that 😄  To assist new contributors I just created #19 and #20

@awaltert perfect thanks! :) 👮
👍

👍

:+1:


Batch Size Issue with RTX 6000 24GB GPU when fine-tuning

Checks

[X] This template is only for question, not feature requests or bug reports.
[X] I have thoroughly reviewed the project documentation and read the related paper(s).
[X] I have searched for existing issues, including closed ones, no similar questions.
[X] I confirm that I am using English to submit this report in order to facilitate communication.

Question details
In the fine_tune_cli.py script, lines 22 to 26, you mentioned that a 24GB GPU can handle a batch size of 3200 samples. However, I am using an RTX 6000 24GB GPU, and it crashes when running fine_tune_cli.py with a batch size of 3200. I have to set the batch size very small for the code to run without any issues. What could be the problem?

Checks

[x]  This template is only for question, not feature requests or bug reports.
[x]  I have thoroughly reviewed the project documentation and read the related paper(s).
[x]  I have searched for existing issues, including closed ones, no similar questions.
[x]  I confirm that I am using English to submit this report in order to facilitate communication.

Question details
In the fine_tune_cli.py script, lines 22 to 26, you mentioned that a 24GB GPU can handle a batch size of 3200 samples. However, I am using an RTX 6000 24GB GPU, and it crashes when running fine_tune_cli.py with a batch size of 3200. I have to set the batch size very small for the code to run without any issues. What could be the problem?

https://github.com/SWivid/F5-TTS/blob/3e73553bd991835979263968fc82fdf1a1022630/src/f5_tts/model/backbones/dit.py#L108
you can set checkpoint_activations=True to save memory, but it will cost more time


in download_redcap.sh script if you have a space in your password then script generate errors

Here the issue

Adding " to  password="$password**"** will resolve the issue.
I've also delete the -v to not have a long list of report:

@hpotier  - thanks for the suggestion.  Do you want to issue a pull request?
https://github.com/aldefouw/redcap_cypress_docker/compare/main...hpotier:redcap_cypress_docker:patch-HP
Can you elaborate upon what the issue is with the verbose flag (-v)?  Is it simply too noisy or is there a technical issue with it?


refactor: remove ternaries from language keys

TODO:

[x] Lots of stuff in Skyra
[x] Finish up https://github.com/skyra-project/settings-gateway/pull/174
[x] Finish up https://github.com/skyra-project/klasa/pull/2
[x] Bump Klasa and SG in lockfile accordingly
[x] something with commandPrunePositions and commandPruneFilters which return a Map
[x] something with all the uses of createPick()

Ternaries removal TODOs:

[x] transform actionAddRole to not use ternaries
[x] transform actionBanReason to not use ternaries
[x] transform actionKickReason to not use ternaries
[x] transform actionMuteReason to not use ternaries
[x] transform actionRemoveRole to not use ternaries
[x] transform actionRestrictedAttachmentReason to not use ternaries
[x] transform actionRestrictedEmbedReason to not use ternaries
[x] transform actionRestrictedReactReason to not use ternaries
[x] transform actionRestrictedVoiceReason to not use ternaries
[x] transform actionSetNickname to not use ternaries
[x] transform actionSetNicknameNoReason to not use ternariess
[x] transform actionSharedRoleSetup to not use ternaries
[x] transform actionSoftbanReason to not use ternaries
[x] transform actionUnbanReason to not use ternaries
[x] transform actionUnmuteReason to not use ternaries
[x] transform actionUnsoftbanReason to not use ternaries
[x] transform actionUnvmuteReason to not use ternaries
[x] transform actionVkickReason to not use ternaries
[x] transform actionVmuteReason to not use ternaries
[x] transform commandAnnouncementEmbedMentions to not use ternaries
[x] transform commandC4GameNext to not use ternaries
[x] transform commandC4GameWin to not use ternaries
[x] transform commandCoinFlipLoseDescription to not use ternaries
[x] transform commandCoinFlipWinDescription to not use ternaries
[x] transform commandDehoistEmbed to not use ternaries
[x] transform commandHungerGamesResultDeaths to not use ternaries
[x] transform commandHungerGamesResultHeader to not use ternaries
[x] transform commandModerationFailed to not use ternaries
[x] transform commandOverwatchEmbedData to not use ternaries
[x] transform commandReasonNotExists to not use ternaries
[x] transform commandWheelOfFortuneCanvasText to not use ternaries
[x] transform commandYarnEmbedDescription to not use ternaries
[x] transform eventsGuildMemberRemoveDescription to not use ternaries
[x] transform eventsGuildMemberUpdateRoles to not use ternaries
[x] transform eventsNameDifference to not use ternaries
[x] transform eventsRoleDifference to not use ternaries
[x] transform giveawayEnded to not use ternaries
[x] transform promptlistMultipleChoice to not use ternaries
[x] transform selfModerationCommandShow to not use ternaries
[x] transform systemExceededLengthOutput to not use ternaries
[x] transform systemExceededLengthOutputConsole to not use ternaries
[x] transform systemExceededLengthOutputFile to not use ternaries
[x] transform systemExceededLengthOutputHastebin to not use ternaries
[x] transform systemGuildMutecreateExceptions to not use ternaries
[x] transform systemGuildMutecreateApplied to not use ternaries
[x] transform commandPermissionNodesShow to not use ternaries

Fixes #1219

Done up until and including commandBlocklistSuccess

@Soumil07 please force push your local version of this branch. Hopefully it's more salvageable than this mess.

Added Needs Testing label to test the changes of merged Klasa and SG PRs


Fix CVE 2019 8331

Updated Bootstrap version to 3.4.1 fixing this security problem.

Coverage remained the same at 100.0% when pulling de1437597cfc45f087ac2678abef5e86cd34d798 on fix_cve-2019-8331 into d841abff083e40d8adfe89a28f39e92e4361b4c4 on release-candidate.


How can I copy what the textureView is displaying and encode it

I am using the textureview display video. This video is from other places. I can't get video data, but I want to encode and stream for each frame displayed in TextureView, how can I get video frame data? Or how to share the frame buffer to the input surface provided by the encoder? Can you give me a sample program?

You have multiple ways to send a video frame to rtmp/rtsp module.
1 - Get raw h264 frames from any source and send it directly to rtmp/rtsp module.
2 - Get a NV21/YUV12 frame and encode it using VideoEncoder class of the library that return you a raw h264. You can send this frame to rtmp/rtsp modules
3 - Render a surface produced by VideoEncoder class that return you a raw h264 frame. You can send this frame to rtmp/rtsp modules
The question is, which one can you do with your video data?
You can read Camera1Base to check this cases:
Case 1:
https://github.com/pedroSG94/rtmp-rtsp-stream-client-java/blob/master/rtplibrary/src/main/java/com/pedro/rtplibrary/base/Camera1Base.java#L880
https://github.com/pedroSG94/rtmp-rtsp-stream-client-java/blob/master/rtplibrary/src/main/java/com/pedro/rtplibrary/rtmp/RtmpCamera1.java#L185
Case 2 (read prepareVideo, startStream and stopStream to know how to use videoEncoder class):
YUV frame provided by Android Camera API:
https://github.com/pedroSG94/rtmp-rtsp-stream-client-java/blob/master/rtplibrary/src/main/java/com/pedro/rtplibrary/base/Camera1Base.java#L894
Case 3 (before send surface you need use prepareVideo):
I send surface provided by VideoEncoder class to a class that render it using OpenGl but you can render that surface by any way to want:
https://github.com/pedroSG94/rtmp-rtsp-stream-client-java/blob/master/rtplibrary/src/main/java/com/pedro/rtplibrary/base/Camera1Base.java#L620

Is there a way to get the video stream frame directly through the surfaceTexture provided by the textureView? Or share the frame buffer to the surface provided by videoEncoder?

Depend of few thing but you can do this:
1 - You can try convert your surfacetexture to surface and get frames with canvas (this maybe not work for you but it is fast to test). This way use CPU so performance increase with better devices but a normal device maybe can handle 15-20 fps in 1080p. This is my recommend way.
You can try with native way if not working as normal way:
https://stackoverflow.com/questions/33692409/how-to-associate-the-existing-surface-to-mediacodec-encoder-surface
I recommend you create a project to do a demo doing a copy to a surface of SurfaceView. After that, you can apply this to my library:
1.1 - Go to camera1 or camera2 base and remove CameraManager class that will be replaced by this canvas render.
1.2 Replace CameraManager funcionality to your new render using OpenGlView (Using OpenGlView you can copy data to surface provided by openglview.getSurface()).
2 - TextureView can get a bitmap that contain a screenshoot of your video frame but it has really bad performance.
The hard way but with better performance:
3 - You can try copy frames to another surface using openGL but you need study this API and it is a bit complex if you never do it. You can use SurfaceManager class provided byt this library to copy frames but you need understand how to create a openGL context and how to use it to work.


Incompatibility between protobufjs and @bufbuild/protobuf

It's currently not possible to load a .proto file with protobufjs and turn it into a descriptor with @bufbuild/protobuf.
So far, debugging lead me to this:

protobufjs does not resolve field type names for message type references to their fully qualified names.
@bufbuild/protobuf however expects those to be fully resolved in newFields https://github.com/bufbuild/protobuf-es/blob/ae29a60c7c6b396da73c14bf858758884845843a/packages/protobuf/src/create-descriptor-set.ts#L537

This leads to error: invalid FieldDescriptorProto: type_name <TYPE NAME> not found
Even adding the fully qualified names in the source .proto doesn't resolve it which leads me to believe that protobufjs actually strips those (expecting them to be resolved by parent lookup in the consumer?)

The "JSON descriptors" protobuf.js uses and produces are conceptually similar to the google/protobuf/descriptor.proto messages, but they are not the same. Unfortunately, they only contain a subset of the information in descriptor.proto, and the format is unique to protobuf.js.
protobufjs/ext/descriptor is an experimental extension to convert the protobuf.js descriptors to protobuf messages.

I see. Are you aware of any alternatives by chance? I'm trying to load raw .proto files at runtime and use them with @bufbuild/proto if at all possibe.

I fear I might_ have to resort to exec with buf build to produce the file descriptor set :-/

I'm not aware of a really awesome solution. I would try to avoid going from protobuf.js descriptors to descriptor.proto to @bufbuild/protobuf messages. Because protobuf.js descriptors are a subset of descriptor.proto, some data needs to be synthesized to produce descriptor.proto, and this is likely to cause problems. I have a hunch that relative references in type names are just the tip of the ice berg.
@bufbuild/protobuf messages only need a subset of the information in descriptor.proto. protobuf.js descriptor might have all necessary data to go from them to @bufbuild/protobuf directly. But I would take a closer look first, especially regarding syntax. (@bufbuild/protobuf makes a distinction between proto2 and proto3 that protobuf.js might just not care about.)
Shelling out to the buf CLI is definitely the most compatible approach, and also the easiest and most performant, if you can run the binary. It works for esbuild too 🙂 With spawn() from node:child_process, you can pipe to descriptors straight into memory.

Thanks. I implemented it using spawn for now. It's not ideal because of the restrictions that puts on portability but it works ;-).
Ty!

That was fast! Did you know we have a rudimentary starting point for a JS parser here? 😇


Rewrite of ethclient.py

I've noticed that the current pyethclient is looking a little rudimentary, so I've rewritten it. I've used Click: it's well documented, looks sophisticated but is easy to use, and I just love decorators.
The subcommands are still the same (I've renamed sign to signtx for consistency, though), but I've added a bunch of documentation (ethclient --help and ethclient [command] --help) and some parameter validation (addresses, private keys, and arbitrary hex strings).
As a side effect, the APIClient has become more seperated from the commands, so I've given it a cleaner interface (it raises exceptions instead of returning errors as dicts and deals with Transaction objects instead of serialized transactions)

looking forward to use a revised version in pyethapp. can you create a pull request there and i'd refactor to work with pyethapp.


Istio-Proxy leaks memory on ServiceEntry updates

Describe the bug
I have a minimal setup of Istio 1.1.0-snapshot.6 with a single Deployment.
When I repeatedly create and delete the same ServiceEntry the memory consumption of the Istio-proxy next to my service only goes up to > 200MB.
Expected behavior
When ServiceEntries are deleted the memory that was claimed before should be freed again.
Steps to reproduce the bug
Install Istio 1.1.0-snapshot.6 and install the following deployment:
apiVersion: apps/v1
kind: Deployment
metadata:
  name: testcurl
  namespace: sr-test
spec:
  selector:
    matchLabels:
      app: testcurl
  replicas: 1
  template:
    metadata:
      labels:
        app: testcurl
    spec:
      containers:
      - name: testcurl
        image: hub.docker.prod.walmart.com/library/openjdk:8-jdk
        command:
          - sleep
          - "100000"
---
apiVersion: v1
kind: Service
metadata:
  name: testcurl
  namespace: sr-test
  labels:
    app: testserver
spec:
  selector:
    app: testcurl
  ports:
    - name: http
      port: 80
      protocol: TCP
      targetPort: 80

Now the memory consumption of the Istio-proxy shows this:
CMD                           RSS
/usr/local/bin/pilot-agent  16376
/usr/local/bin/envoy -c /et 31876

Now create a file with this ServiceEntry:
apiVersion: networking.istio.io/v1alpha3
kind: ServiceEntry
metadata:
  name: wmt-subnets
  namespace: default
spec:
  hosts:
  - "*.example.com"
  addresses:
  - <IP_ADDRESS>/19
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/18
  - <IP_ADDRESS>/18
  - <IP_ADDRESS>/18
  - <IP_ADDRESS>/18
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  - <IP_ADDRESS>/16
  ports:
  - number: 3306
    name: crdbms
    protocol: TCP
  - number: 7000
    name: csdr1
    protocol: TCP
  - number: 7199
    name: csdr2
    protocol: TCP
  - number: 9042
    name: csdr3
    protocol: TCP
  - number: 9160
    name: csdr4
    protocol: TCP
  - number: 9142
    name: csdr5
    protocol: TCP
  - number: 60000
    name: csdr6
    protocol: TCP
  - number: 9000
    name: kafka1
    protocol: TCP
  - number: 9091
    name: kafka2
    protocol: TCP
  - number: 9092
    name: kafka3
    protocol: TCP
  - number: 9093
    name: kafka4
    protocol: TCP
  - number: 8983
    name: solr
    protocol: TCP
  - number: 8000
    name: splunk
    protocol: HTTP
  resolution: NONE
  location: MESH_EXTERNAL

(I admit that this might not be the smallest ServiceEntry, but I experience the same behavior at a smaller scale with smaller, more conventional, ServiceEntries.)
Now repeatedly install and remove this ServiceEntry.
After some installs the memory consumption of the Istio-Proxy jumps by ~25MB and this is not returned anymore.
I don't send any requests, so the proxy basically has nothing to do from my perspective.
While removing and adding the ServiceEntry I see these memory consumptions:
CMD                           RSS
/usr/local/bin/pilot-agent  16376
/usr/local/bin/envoy -c /et 59712

CMD                           RSS
/usr/local/bin/pilot-agent  16880
/usr/local/bin/envoy -c /et 84848

CMD                           RSS
/usr/local/bin/pilot-agent  16880
/usr/local/bin/envoy -c /et 111248

CMD                           RSS
/usr/local/bin/pilot-agent  16880
/usr/local/bin/envoy -c /et 137912

CMD                           RSS
/usr/local/bin/pilot-agent  16880
/usr/local/bin/envoy -c /et 164312

Even after ~20 minutes the memory consumption is the same.
Version
Kubernetes version 1.10.11 on Docker for Mac.
Istio 1.1.0-snapshot.6
Installation
helm install ~/istio/istio-1.1.0-snapshot.6/install/kubernetes/helm/istio-init --name istio-init --namespace istio-system -f ~/istio/istio-1.1.0-snapshot.6/install/kubernetes/helm/istio/values-istio-auth-mcp.yaml  --set global.mtls.enabled=true --set global.tracing.enabled=true --set pilot.traceSampling=100.0 --set tracing.enabled=true --set grafana.enabled=true --set global.useMCP=false
helm install ~/istio/istio-1.1.0-snapshot.6/install/kubernetes/helm/istio --name istio --namespace istio-system -f ~/istio/istio-1.1.0-snapshot.6/install/kubernetes/helm/istio/values-istio-auth-mcp.yaml  --set global.mtls.enabled=true --set global.tracing.enabled=true --set pilot.traceSampling=100.0 --set tracing.enabled=true --set grafana.enabled=true --set global.useMCP=false
Environment
Running locally on Docker for Mac
Cluster state
The script for the cluster state fails on the docker-compose container, so I can't create whatever is expected.
However here is what I could get manually:
clusterinfo.zip
Also the logs of galley, pilot and the proxy:
logs.zip

Just retested with 1.1.0-rc.0 and it seems like this is fixed.

Reopening again, I didn't realize that after the installation of 1.1.0-rc.0 pilot didn't start due to the resource requests.
Memory consumption seems to rise slower now, but I easily got to >100MB, and that with a single ServiceEntry!

What even worth is, if keep deleting/adding serviceentry, ultimately the envoy process within the istio-proxy will be restarted.
This can be verified by running the following script:
for i in $(seq 1 100); do echo "round $i: add servcieentry"; kubectl apply -f se_test.yml; sleep 10; echo "delete servcieentry";kubectl -n dep-se-test delete serviceentries.networking.istio.io wmt-subnets; kubectl -n dep-se-test exec -it testcurl-85c4c6bd47-xcw97 -c istio-proxy -- ps -e -o pid,ppid,rss,comm --sort -rss; echo ""; done
This happened in 1.10-rc.0

@silentdai could you take a look at this?  I'm not sure if it's in the mixer filter or envoy itself.  I think it's in envoy.
CC @costinm @PiotrSikora

Could you check and/or upload config dump from the proxy, before and after? I'm wondering if the add/remove dance results only in new entries being added to the proxy, but never removed:
kubectl exec -it pod -c istio-proxy -- curl -s <IP_ADDRESS>:15000/config_dump > /tmp/config_dump

Also, Envoy uses tcmalloc, so maybe some of this memory is freed, but not released back to the operating system:
kubectl exec -it pod -c istio-proxy -- curl -s <IP_ADDRESS>:15000/stats | grep server.memory

Thanks!

@robertpanzer Thank you for the detailed report! I will take a look today.
@PiotrSikora Thanks for the comment!

My experiment shows that RSS or server.memory usage was increasing but ceased some how.
https://gist.github.com/silentdai/0e887c3c0452a848cc0e788d24bbab73
I don't have theory so far. Will continue investigation tomorrow.

This is what I collected 10 hours after the previous comment. https://gist.github.com/silentdai/97f80b3ae47543b668b2c4ea2e0993f7#file-gistfile1-txt-L35-L36
server.memory_allocated: 7097904
server.memory_heap_size: 22020096

envoy returned the allocated memory to malloc system (18505040 -> 7097904) while the heap size remains the same (22020096 -> 22020096) as tcmalloc doesn't return memory to OS.
So far the conclusion is that envoy doesn't leak at my side. Maybe I didn't fully reproduce.
@robertpanzer  how frequent were you applying service entry changes?

@silentdai I deleted and created the ServiceEntry at an interval of maybe 5 seconds and reached >100 MB rss after maybe 10 or 20 creates.
This was certainly only to reproduce the bug.
I figured earlier that my services started becoming unresponsive due to the high memory consumption and that was the smallest and fastest reproducer that I could find.
What I want to say is that my approach doesn't directly reflect a realistic deployment history, but it occurred at a slower pace on my local dev machine where I only have 2 or 3 services running, so for clusters I would see this to be even worse.

@robertpanzer You mentioned you were using istio1.1 snapshot6. Could you verify it again that we deployed the same envoy version by the below command?
$ kubectl exec -it <PODNAME> -c istio-proxy -- curl -s <IP_ADDRESS>:15000/server_info | grep version
 "version": "ea5e8afb79fc1864c244d397192e06513b918f49/1.10.0-dev/Clean/RELEASE/BoringSSL"

I find that envoy doesn't provide heap profling entry while cpu profiling method is there.
With the future patch we do something like below

curl -X POST "localhost:15000/heapprofiler?enable=y"  # currently envoy only provides /cpuprofiler
add/delete service entries
curl -X POST "localhost:15000/heapprofiler?enable=n"
export the profling data and find out what code path and what objects were allocated in between.

@PiotrSikora Is it because no one attempted to work on it? Is there substitution heap profiling method? If not I can provide a simple patch tomorrow so that we enable the above flow.

I just retested it with 1.1.0-rc.0 and got this info:
"version": "fc273e117a6ed875c7cb9b3c3251c8d982bc7196/1.10.0-dev/Clean/RELEASE/BoringSSL"

After 21 create and delete cycles the memory consumption of the istio-proxy jumped to >100MB:
/usr/local/bin/pilot-agent  18472
/usr/local/bin/envoy -c /et 106716

Without any further configurations it seems to stay stable there.
I am not sure if it considers the memory request of 128Mi.

Good to know envoy is not deteriorating beyond 128Mi in this case.  And now we are on the same page: somehow the peak usage is as large as 128Mi.
BTW could you run "curl -L localhost:15000/stats | grep memory"?
We can learn if 106Mi is fully utilized, or reserved by tcmalloc. It's not critical to me at this moment. However, as the investigation goes on, fighting reserved memory and locating how 106Mi memory is fully allocated would diverge IMHO

Dropping to P1 to cover remaining investigation since it looks like 1.1 rc0 does not have a memory leak

I just retried the sequence again after a fresh reinstall of 1.1.0-rc.0 and the problem looks actually worse than I thought.
The memory footprint seems to jump very fast again, up to 30MB per re-apply.
Plus the istio-proxy gets killed as soon as it hits the requested resources of 128Mi.
2019-02-27T14:43:29.730724Z     warn    Epoch 0 terminated with an error: signal: killed
2019-02-27T14:43:29.730762Z     warn    Aborted all epochs
2019-02-27T14:43:29.730796Z     info    Epoch 0: set retry delay to 400ms, budget to 8
2019-02-27T14:43:30.008196Z     info    Envoy proxy is NOT ready: failed retrieving Envoy stats: Get http://<IP_ADDRESS>:15000/stats?usedonly: dial tcp <IP_ADDRESS>:15000: connect: connection
refused
2019-02-27T14:43:30.131888Z     info    Reconciling retry (budget 8)
2019-02-27T14:43:30.132095Z     info    Epoch 0 starting
2019-02-27T14:43:30.141430Z     info    Envoy command: [-c /etc/istio/proxy/envoy-rev0.json --restart-epoch 0 --drain-time-s 45 --parent-shutdown-time-s 60 --service-cluster testcurl.sr-

The curl command gave me this:
server.memory_allocated: 79505608
server.memory_heap_size: 83886080

Also checked that the version of the istio-proxy is correct, it still says:
 "version": "fc273e117a6ed875c7cb9b3c3251c8d982bc7196/1.10.0-dev/Clean/RELEASE/BoringSSL",

In my last test I managed to bring the memory size to 90 MB after about 4 or 5 create/delete cycles.

@robertpanzer but is this a leak where memory grows without bound or does it stabilize?  If the later, are you saying that memory usage is too high?

In my tests today the memory usage didn’t exceed 128MB, but it didn’t really stabilize.
Instead the envoy process got killed every few create/delete cycles of my service entry.
I wonder what that means for accepting and handling requests?
I think this is not a hot restart that could potentially continue in flight requests, is it?

@silentdai @PiotrSikora I think we need to set this back to P0.   We need to understand the relationship between what @robertpanzer is trying to do and Envoy's memory usage.   It's possible that we need to recommend an increased memory limit, but there should be a good explanation for why this is necessary.

It's only 1 step away from merging this PR in the upstream envoy and back port to istio/envoy. Once we have that it's should be easy to determine which path eats up the memory

@silentdai very cool work on adding the heap profiler to envoy.
@robertpanzer my guess is we'll end up resolving this in an early 1.1.x release.  We probably will not pick up a new version of Envoy until immediately after 1.1 ships.

The progress:

It's a known issue that adding and removing service entry(service entry is mapping to envoy proxy listener) will cause heap usage spike. At my side I don't see a leak as it drop down once the service entry is stable. There are attempts to smooth the spike. More is on the way.
Another issue addressed here is envoy restart with no container restart. I don't why the nuance matters. @robertpanzer  Can you explain?

@silentdai I don't think that this nuance is important.
I just think that it is not good the Istio-proxy gets killed on config updates.
There were no requests processed by the proxy at all in my tests, but if I am not wrong I guess that in flight requests would get lost as this doesn't seem to be a hot restart.
Having said that, it's probably nicer that the process gets restarted because it could be a chance for doing a hot restart, but afaict from the logs these look differently.

Should be fixed by https://github.com/envoyproxy/envoy/issues/6252 in istio 1.1.0 rc4
Turning the endpoint on and off is supposed to cause no listener flipping. However, before the fix the unstable marshaling is causing envoy proxy to wrongly determine as config change in listener and expensive listener draining.
The drain would last minutes. When the listener config is pushing frequently, we can expect many draining listener are hanging.
The 4 factors each is linear to the heap usage

number of listeners (at least 13 in above config)
per listener heap usage (~ 1MiB on my config)
draining window (1 minute probably)
listener push frequency (both adding and delete counts)

So if you are flipping 5 times in a draining window, I would expect  5 * 2 * 1 MiB * 13 = 130 MiB heap spike. Also CPU is high because listener spawn is cpu-intensive.
@robertpanzer  Please use istio 1.1 rc4 to retest. My understanding is that the core fix is in istio-pilot although you encountered issue at envoy. However, I highly suggest you upgrade the whole istio system.

1.1rc4 will be created March 12th

I can still reproduce the restarts of the istio-proxy as long as I delete/create within the drain time window, which is indeed 45 seconds.
If I wait for more than 1 minute between delete and the next create memory seems to be reused.
What indeed got better with rc.4 is that if I make small updates to the ServiceEntry, only the changed listeners seem to get drained and recreated.

Your described behavior is expected. As in rc4 the affected listeners are less than before, the memory spike is should be highly reduced. Is rc4 acceptable from your perspective?

@silentdai Can you explain though why the Envoy process restarts instead of the pod?   In other words, what sort of xDS updates would cause Envoy to restart?

I cannot reproduce the envoy process restart without pod restart. Maybe "envoy process restart" was referring to the fact that pod restart?

My observation is more horrible: If the envoy process died, for example, killed by killall envoy.
The envoy process no longer restart. The istio-proxy is alive while the pod is not serving any app traffic. By app traffic I mean book-review http request in the demo.
The reason is that pilot-agent is the main process of istio-proxy

pilot agent is alive so the pod is not restarted
dead envoy won't be restarted by pilot-agent, envoy process is dead forever until pod restart

@costinm ^^ What is the expected consequence if envoy die silently?

I just tested again, and for me only the Istio-proxy process dies, the container and therefore pilot-agent stays alive. I also checked the pod status and the container restartCount is still 0.
In the logs of istio-proxy I see this when the istio-proxy is killed:
2019-03-15T09:03:56.220651Z     warn    Epoch 0 terminated with an error: signal: killed
2019-03-15T09:03:56.220715Z     warn    Aborted all epochs
2019-03-15T09:03:56.220839Z     info    Epoch 0: set retry delay to 200ms, budget to 9
2019-03-15T09:03:56.421053Z     info    Reconciling retry (budget 9)
2019-03-15T09:03:56.421200Z     info    Epoch 0 starting
2019-03-15T09:03:56.422899Z     info    Envoy command: [-c /etc/istio/proxy/envoy-rev0.json --restart-epoch 0 --drain-time-s 45 --parent-shutdown-time-s 60 --service-cluster testcurl.sr-test --service-node sidecar~<IP_ADDRESS>~testcurl-7c7454f99c-l6hhp.sr-test~sr-test.svc.cluster.local --max-obj-name-len 189 --allow-unknown-fields -l warning --concurrency 2]

For me the restarted envoy is serving traffic again.
I guess though that inflight requests will get lost.
In my console I found these logs that could give a hint why Envoy is killed:
(It's Docker for Mac, so probably not representative for a real cluster implementation, but what is a real cluster implementation anyway?)
default	10:35:16.272078 +0100	com.docker.hyperkit	[ 6004.061715] envoy invoked oom-killer: gfp_mask=0x24000c0(GFP_KERNEL), nodemask=0, order=0, oom_score_adj=968
default	10:35:16.274248 +0100	com.docker.hyperkit	[ 6004.064176] envoy cpuset=224bf68c6d39ea20a1b315ba740fecde15d5433cb79bb6611e37a73598be7d20 mems_allowed=0
default	10:35:16.275401 +0100	com.docker.hyperkit	[ 6004.065933] CPU: 2 PID: 28933 Comm: envoy Not tainted 4.9.125-linuxkit #1
default	10:35:16.276221 +0100	com.docker.hyperkit	[ 6004.067075] Hardware name:  BHYVE, BIOS 1.00 03/14/2014
default	10:35:16.277467 +0100	com.docker.hyperkit	[ 6004.067893]<PHONE_NUMBER>000000 ffffffffb1420789 ffffa2048bd73de8 ffff942d049fc240
default	10:35:16.278788 +0100	com.docker.hyperkit	[ 6004.069137]  ffffffffb11f8d5e<PHONE_NUMBER>000000<PHONE_NUMBER>0003c8 ffffa2048bd73de8
default	10:35:16.280023 +0100	com.docker.hyperkit	[ 6004.070466]  ffff942d090fa2a0<PHONE_NUMBER>000202 ffffffffb17e71ce ffff942d049fc240
default	10:35:16.280434 +0100	com.docker.hyperkit	[ 6004.071699] Call Trace:
default	10:35:16.281261 +0100	com.docker.hyperkit	[ 6004.072109]  [<ffffffffb1420789>] ? dump_stack+0x5a/0x6f
default	10:35:16.282109 +0100	com.docker.hyperkit	[ 6004.072933]  [<ffffffffb11f8d5e>] ? dump_header+0x78/0x1ed
default	10:35:16.283152 +0100	com.docker.hyperkit	[ 6004.073780]  [<ffffffffb17e71ce>] ? _raw_spin_unlock_irqrestore+0x16/0x18
default	10:35:16.284064 +0100	com.docker.hyperkit	[ 6004.074823]  [<ffffffffb11a45b5>] ? oom_kill_process+0x83/0x324
default	10:35:16.284948 +0100	com.docker.hyperkit	[ 6004.075735]  [<ffffffffb11a4bdc>] ? out_of_memory+0x239/0x267
default	10:35:16.285944 +0100	com.docker.hyperkit	[ 6004.076619]  [<ffffffffb11f22ac>] ? mem_cgroup_out_of_memory+0x4b/0x79
default	10:35:16.286991 +0100	com.docker.hyperkit	[ 6004.077615]  [<ffffffffb11f646d>] ? mem_cgroup_oom_synchronize+0x26b/0x294
default	10:35:16.288019 +0100	com.docker.hyperkit	[ 6004.078662]  [<ffffffffb11f25f4>] ? mem_cgroup_is_descendant+0x48/0x48
default	10:35:16.288993 +0100	com.docker.hyperkit	[ 6004.079707]  [<ffffffffb11a4c37>] ? pagefault_out_of_memory+0x2d/0x6f
default	10:35:16.289877 +0100	com.docker.hyperkit	[ 6004.080665]  [<ffffffffb10467af>] ? __do_page_fault+0x3c6/0x45f
default	10:35:16.290654 +0100	com.docker.hyperkit	[ 6004.081547]  [<ffffffffb17e8808>] ? page_fault+0x28/0x30
default	10:35:16.294938 +0100	com.docker.hyperkit	[ 6004.082395] Task in /kubepods/kubepods/burstable/pod6e836375-4700-11e9-8430-025000000001/224bf68c6d39ea20a1b315ba740fecde15d5433cb79bb6611e37a73598be7d20 killed as a result of limit of /kubepods/kubepods/burstable/pod6e836375-4700-11e9-8430-025000000001/224bf68c6d39ea20a1b315ba740fecde15d5433cb79bb6611e37a73598be7d20
default	10:35:16.295813 +0100	com.docker.hyperkit	[ 6004.086630] memory: usage 131060kB, limit 131072kB, failcnt 0
default	10:35:16.296751 +0100	com.docker.hyperkit	[ 6004.087481] memory+swap: usage 131072kB, limit 131072kB, failcnt 132
default	10:35:16.297682 +0100	com.docker.hyperkit	[ 6004.088419] kmem: usage 1220kB, limit<PHONE_NUMBER>740988kB, failcnt 0
default	10:35:16.302508 +0100	com.docker.hyperkit	[ 6004.089350] Memory cgroup stats for /kubepods/kubepods/burstable/pod6e836375-4700-11e9-8430-025000000001/224bf68c6d39ea20a1b315ba740fecde15d5433cb79bb6611e37a73598be7d20: cache:4780KB rss:125060KB rss_huge:0KB mapped_file:4772KB dirty:0KB writeback:0KB swap:12KB inactive_anon:46272KB active_anon:83564KB inactive_file:0KB active_file:0KB unevictable:0KB
default	10:35:16.303741 +0100	com.docker.hyperkit	[ 6004.094175] [ pid ]   uid  tgid total_vm      rss nr_ptes nr_pmds swapents oom_score_adj name
default	10:35:16.305198 +0100	com.docker.hyperkit	[ 6004.095539] [27056]  1337 27056     8118     4465      20       5        3           968 pilot-agent
default	10:35:16.306448 +0100	com.docker.hyperkit	[ 6004.096867] [28933]  1337 28933    60704    34827      89       3        0           968 envoy
default	10:35:16.307737 +0100	com.docker.hyperkit	[ 6004.098117] Memory cgroup out of memory: Kill process 28933 (envoy) score 2010 or sacrifice child
default	10:35:16.309314 +0100	com.docker.hyperkit	[ 6004.099494] Killed process 28933 (envoy) total-vm:242816kB, anon-rss:119924kB, file-rss:14612kB, shmem-rss:4772kB

My interpretation is that the operating system simply kills the process because it's cgroup memory limit is exceeded.
And indeed the configuration seems to support this thesis:
istio-proxy@testcurl-7c7454f99c-l6hhp:/$ cat /sys/fs/cgroup/memory/memory.limit_in_bytes
134217728
istio-proxy@testcurl-7c7454f99c-l6hhp:/$ cat /sys/fs/cgroup/memory/memory.oom_control
oom_kill_disable 0
under_oom 0

Pilot Agent is probably not killed because killing envoy is sufficient to free enough memory.
The OOM killer seems to be selective:
From https://lwn.net/Articles/761118/

To simplify a bit: when the OOM killer is invoked, it tries to pick the process whose demise will free the most memory while causing the least misery for users of the system.

I can imagine though that other Kubernetes/Container runtime implementations work differently and result in the behavior observed by @silentdai

@robertpanzer
Let's use "envoy" to specify the process and "istio-proxy" to specify the container. With your detailed the description I can understand this time :)
So we have understood that it's the oom killer that terminate envoy process. We won't try to reproduce other logic causing envoy died.

Are you still using the service entry config rotating delete and apply?
It's a known problem that envoy could cause heap spike during listener operation (up and down), as calculated above. IMHO adding or deleting service entry is rare in production. So is the issue currently blocking you, or you are fine leave it open just for tracking?

I was just wondering how the process was killed, and it looked to me like I was not alone with this :-D
To find out what happens I still did the repeating create and delete.
I agree that this should not happen too often, and it's not blocking me anymore.

Actionable here and close this issue:

@robertpanzer Please don't create and delete service entry point too frequently. It's known problem and not high priority as we shouldn't see this in production. In case of emergency we can reduce the drain window to mitigate.
I confirm the envoy won't be restarted if I kill it. The pod is not dying. Maybe there is health check setting could mitigate it. I will look into pilot-agent to see if envoy can be spawn up again by pilot-agent.
If the envoy dies, the request from app is supposed to be lost. It should be handled by application. No one can guarantee 0 bug at envoy. However, as state as above, pilot-agent(or health check?) should not leave the sidecar pod in the state that envoy die for ever.

All,
I am still experiencing this issue on 1.1.1 installed to OKD 3.11.
Sidecar with 128mb limit was crashing randomly every few minutes with OOM.
Increased limit to 256mb and sidecar is now reporting using 200mb soon after the start.
In our case, we are not adding any new Istio rules. It all happens with no changes to Istio config.
What we do not understand is why memory footprint is so high in our case- seems to but much higher than expected in "standard" deployment.

Sorry for hijacking this issue but we actually use dynamic service entry creation / deletion in production and have been bitten by this:

We provide a platform on which customer can spin up instances.
We check if these instances can receive traffic from outside the cluster by creating a service entry and then attempting to access the host from within the cluster.

We could drop this check or remove the service doing the check from the service mesh but going forward we planned on using service entries heavily by providing APIs for whitelisting outbound traffic from these instances. Is this something we should plan to implement otherwise?

@jschlichtholz  Sorry for the late reply. It highly depends on how frequent your collection service endpoints change. However, I would always suggest providing a pool that member never change(or changes less infrequently) and do not add/remove member upon health check response. See the envoy philosophy here


[FEATURE] Add documentation about --log-file usage

Is your feature request related to a problem? Please describe.
The current documentation does not have any information about using a separate log file.
Describe the solution you'd like
Provide information on how someone can use the --log-file parameter.
Describe alternatives you've considered
N.A.
Additional context
N.A.

@benmalef can you take a crack at this? Should be simple enough.


Disable travis ci cache

Summary
We've been burned in the past with cached node modules not catching a current problem. Lets try disabling the cache and see how it affects build times.
Thanks for contributing to Terra.
@cerner/terra

There is a lint error that showed up recently:
/home/travis/build/cerner/terra-core/packages/terra-markdown/src/Markdown.jsx
  4:8  error  Missing file extension "css" for "github-markdown-css"  import/extensions

I'm working around this in the release prep by disabling the linter.
https://github.com/cerner/terra-core/commit/242665ed07a9b47590925259c163dc9ee4bef1d3
It is erroneous, as webpack does resolve this correctly. The github-markdown-css package.json points its main entry to a CSS file and the linter seems to be getting caught up on this. Though I checked the generated CSS and the file is imported from the github-markdown-css package correctly.
We could probably move away from using the github-markdown-css package and just copy/paste their styles into a CSS file and import them ourselves with the file extension.

The changes in time seem to be negligible. I propose we disable caching.
Cacheing Disabled:

Master Baseline:


Fix ownership mistakes in IVFFlatIndexImpl

Hmm true, back then we had agreed to always own the inner index, otherwise it could get messy. I imagine these functions just slipped through review?
If this is the case and there is nothing else to improve here, then I suppose it would be good to merge.

PR with these functions was merged earlier than PR with "always own".
I found this difference by chance today.


Set language name as class to CM display wrapper - enable mode specific code coloring

This will enable theme developers and contributors to design code themes for a particular doc mode by using descendant selectors.
For example consider the following HTML snippet -
 <div>
     My Sample content
 </div>

And the following CSS snippet -
 div {
     color: #00f;
 }

In both HTML and CSS doc modes 'div' has been assigned '.cm-tag' selector.
This implies that if in this LESS file the selector group containing '.cm-tag' is assigned
a color value '#446fbd', the tag name 'div' in HTML and the selector name 'div' in CSS will
have the same color.
However it's possible to specify different color values for these selectors targetting
different language modes. What needs to be done is identify the tokens what's being used
in a particular doc mode and target the corresponding selectors by nesting them under the
MODE selector.
For example let's take the snippets and case described above. If we want to override the
default color of tag only in HTML mode, we need to append this selector after the "custom" marker
in this LESS file.
 .HTML {
      .cm-tag {
          color: <custom color here>;
      }
 }

This example can be expanded to include individual selectors or certain selectors grouped together.
These are the list of popular language modes along with the file extensions which they are associated with. Any of these modes can be used as parent selectors and then the individual token color can be
nested inside to make sure that we change only the targetted modes coloring.
->Groovy(groovy,gradle)
->Properties(ini,properties)
->CSS(css,css.erb)
->SCSS(scss,scss.erb)
->Stylus(styl)
->JavaScript(js,js.erb,jsm,_js)
->JSON(json)
->VBScript(vbs)
->VB(vb)
->XML(xml,wxs,wxl,wsdl,rss,atom,rdf,xslt,xsl,xul,xsd,xbl,mathml,config,plist,xaml)
->SVG(svg)
->HTML(html,htm,shtm,shtml,xhtml,cfm,cfml,cfc,dhtml,xht,tpl,twig,kit,jsp,aspx,ascx,asp,master,cshtml,vbhtml)
->EJS(ejs,dust)
->Embedded-Ruby(erb,html.erb,htm.erb)
->JSX(jsx)
->C(c,h,i)
->C++(cc,cp,cpp,c++,cxx,hh,hpp,hxx,h++,ii,ino)
->C#(cs,asax,ashx)
->Java(java)
->Scala(scala,sbt)
->Dart(dart)
->PHP(php,php3,php4,php5,phtm,phtml,ctp)
->CoffeeScript(coffee,cf,cson,_coffee)
->Clojure(clj,cljs,cljx)
->Perl(pl,pm,t)
->Ruby(rb,ru,gemspec,rake)
->Python(py,pyw,wsgi,gyp,gypi)
->SASS(sass)
->Diff(diff,patch)
->Lua(lua)
->YAML(yaml,yml)
->SQL(sql)
->Haxe(hx)
->Bash(sh,command,bash)
->Haskell(hs)
->RDF-Turtle(ttl)
->Markdown(md,markdown,mdown,mkdn)
->Markdown-(GitHub)
->LESS(less)
->Handlebars(hbs,handlebars)

Is the addModeClass option not enough?

@ficristo Thanks a lot for pointing it out 👍. I needed this for JavaScript specific code coloring but missed the 'addModeClass' option completely.  Closing this PR, I will enable the option in theme config json.


More optimizations, remove history preservation

Implement some more of the identified optimizations:
- ReluctantQuant 176ms
- ReluctantQuantWithTerminal 231ms
- EagarQuantWithTerminal 69.1ms
- BasicBacktrack 20.5ms
- BasicBacktrackFirstMatch 19.9ms
- cssRegex 93.6ms
- FirstMatch 140ms
- AllMatches 37.1ms

After

- ReluctantQuant 117ms
- ReluctantQuantWithTerminal 75ms
- EagarQuantWithTerminal 68.3ms
- BasicBacktrack 10ms
- BasicBacktrackFirstMatch 10.1ms
- cssRegex 41.4ms
- FirstMatch 76.8ms
- AllMatches 38ms

@swift-ci please test

Merging to unblock some more perf work


Added cast to type support

Added support for casting to type; e.g.
assign a = int'(some_var);
assign b = my_type_def'(some_var);

I do have a few tests.
How do you want me to include them in the pull request?
Richard
From: Catherine @.>
Date: Friday, 15 March 2024 at 11:59
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>, Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
@whitequark commented on this pull request.
No tests?
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-1940117522, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCILLFQNPE7SK2HI4WLYYNAIRAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBQGEYTONJSGI.
You are receiving this because you authored the thread.Message ID: @.***>

Tests added to the tests/verilog directory
Richard
From: Richard Herveille @.>
Date: Friday, 15 March 2024 at 14:13
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.***>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
I do have a few tests.
How do you want me to include them in the pull request?
Richard
From: Catherine @.>
Date: Friday, 15 March 2024 at 11:59
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>, Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
@whitequark commented on this pull request.
No tests?
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-1940117522, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCILLFQNPE7SK2HI4WLYYNAIRAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBQGEYTONJSGI.
You are receiving this because you authored the thread.Message ID: @.***>

Done
On Mar 17, 2024, at 09:49, Catherine @.***> wrote:

@whitequark commented on this pull request.
Could you also add a test for a cast to a user defined struct please?
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-1941530653, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCLIJIL4A42HIUA4FQTYYXCSBAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBRGUZTANRVGM.
You are receiving this because you authored the thread.Message ID: @.***>

Hi Zach,
See my answers inline.

I think this patch is already >70% of the work needed to also support:

Ø  x = (struct_t'(y)).z;
Ø  I'm sure we'd gladly accept a patch adding support if you're interested!
Yes, but I am working on enhancing ‘import’ first.
I also noticed that my_struct_variable.my_struct_variable.my_struct_variable is not supported. That’s on the todo list as well.
Cheers,
Richard
From: Zachary Snow @.>
Date: Wednesday, 22 May 2024 at 17:55
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>, Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
@zachjs requested changes on this pull request.
I think substantively this change is in a great state. I think nearly all of my comments are minor or stylistic. Sorry for the delay in getting this reviewed!
I am excited for Yosys to gain this feature! Thank you for your contribution!
I think this patch is already >70% of the work needed to also support:
x = (struct_t'(y)).z;
I'm sure we'd gladly accept a patch adding support if you're interested!

In frontends/ast/ast.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610806809:

       switch (children[0]->type)

         {

         case AST_WIRE:

                 if (children[0]->children.size() > 0) {

                         children[0]->children[0]->dumpVlog(f, "");

                         fprintf(f, "'(");

                         children[1]->dumpVlog(f, "");

                         fprintf(f, ")");

                 } else {

                         fprintf(f, "%d'(", children[0]->range_left - children[0]->range_right +1);

                         children[1]->dumpVlog(f, "");

                         fprintf(f, ")");

                 }

                 break;

         default: //AST_IDENTIFIER

I assume this branch covers at least AST_CONSTANT, too. What am I missing?
[rih] The default statement covers that. It was already supported, I didn’t change any of that code.
I merely added AST_WIRE

In frontends/ast/ast.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610809183:

               fprintf(f, "'(");

                 children[1]->dumpVlog(f, "");

                 fprintf(f, ")");

Arguably these three lines are now repeated three times. Can we do this once at the end instead?
[rih] Done

In frontends/ast/ast.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610809746:

         break;

There is a stray newline here.

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610809776:

@@ -1487,6 +1487,7 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin

                   width_hint = max(width_hint, children[1]->range_left - children[1]->range_right + 1);

           }

           break;

There is a stray newline here.
[rih] No, a newline was missing in the original file

In tests/verilog/size_cast.svhttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610810631:

@@ -15,6 +15,19 @@ module top;

 logic signed [1:0] L2sb10 = 2;

 logic signed [1:0] L2sb11 = 3;

typedef logic              u1bit_t;

typedef logic signed       s1bit_t;

typedef logic        [1:0] u2bit_t;

typedef logic signed [1:0] s2bit_t;

typedef logic        [2:0] u3bit_t;

typedef struct packed {

      u1bit_t sign;

      u3bit_t msbs;

      byte    lsbs;

} s12bit_packed_struct_t;

When I created this test, I don't think I intended to have any double-newlines breaking up portions of the test. Would you mind sticking to the existing style here?
[rih] Does it matter? People use double-newlines to create blocks in Verilog code all the time.
It helped me to understand which tests were related.
But I removed the double newlines, because you asked.

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610813480:

                                //delete child's children

I think it's best to avoid comments that simply turn a line of code into a sentence. For example, I don't think //delete child's children clarifies child->delete_children(); at all.
⬇ Suggested change

                                //delete child's children

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816272:

                                //create AST_CONSTANT node

⬇ Suggested change

                                //create AST_CONSTANT node

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816402:

                         // Ensure typedef itself is fully simplified

                         while (template_node->simplify(const_fold, stage, width_hint, sign_hint)) {};

                         switch (template_node->type)

                         {

                         case AST_WIRE: {

                                if (template_node->children.size() > 0 && template_node->children[0]->type == AST_RANGE)

                                        width = range_width(this, template_node->children[0]);

                                //delete child's children

                                child->delete_children();

                                //create AST_CONSTANT node

                                node = mkconst_int(width, true);

⬇ Suggested change

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816615:

                       switch (template_node->type)

                         {

                         case AST_WIRE: {

                                if (template_node->children.size() > 0 && template_node->children[0]->type == AST_RANGE)

                                        width = range_width(this, template_node->children[0]);

                                //delete child's children

                                child->delete_children();

                                //create AST_CONSTANT node

                                node = mkconst_int(width, true);

                                break;

                         }

                         case AST_STRUCT: {

Similar changes in this block.

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816835:

                 //insert AST_CONSTANT

⬇ Suggested change

                 //insert AST_CONSTANT

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816911:

                              //delete child's children

                                child->delete_children();

                                //create AST_CONSTANT node

                                width = size_packed_struct(template_node, 0);

                                node = mkconst_int(width, false);

                                break;

                         }

                         default:

                                log_error("Don't know how to translate static cast of type %s\n", type2str(template_node->type).c_str());

                         }

                 }

                 //Remove child node

⬇ Suggested change

                 //Remove child node

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610819253:

                       default:

                                log_error("Don't know how to translate static cast of type %s\n", type2str(template_node->type).c_str());

                         }

                 }

                 //Remove child node

                 delete child;

                 children.erase(children.begin());

                 //insert AST_CONSTANT

                 children.insert(children.begin(),node);

         }

         detect_width_simple = true;

         children_are_self_determined = true;

⬇ Suggested change

I recognize comments like these are pretty nit-picky! That said, I think it's important to adhere roughly to the existing style given that Yosys has a relatively broad base of contributors.
[rih] I find a lot of the code seriously lacking in documentation. Especially these complex structures.
But that’s a personal preference.

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610819580:

                         //Construct AST_CONSTANT node

⬇ Suggested change

                         //Construct AST_CONSTANT node

—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-2072606221, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCM6ZHDXXAT6L2NRD2LZDU46BAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZSGYYDMMRSGE.
You are receiving this because you authored the thread.Message ID: @.***>

Any update on the pull request?
All requested changes have been implemented.

To short-circuit another round of reviews and edits, I've made some edits locally. I'd like to add them to this PR and have you take a look before I merge them. Can you either enable "Maintainers are allowed to edit this pull request." or pull in my changes at https://github.com/zachjs/yosys/tree/cast_type?

I am not using a personal account. Github doesn’t allows "Maintainers are allowed to edit this pull request." To non-personal accounts.
I will need to find time to pull in your changes. But time is currently extremely limited.
Richard
From: Zachary Snow @.>
Date: Sunday, 1 September 2024 at 06:50
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>, Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
To short-circuit another round of reviews and edits, I've made some edits locally. I'd like to add them to this PR and have you take a look before I merge them. Can you either enable "Maintainers are allowed to edit this pull request." or pull in my changes at https://github.com/zachjs/yosys/tree/cast_type?
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2323348635, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCNNONLUYF3KDNXZOSLZUMLR5AVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGM2DQNRTGU.
You are receiving this because you authored the thread.Message ID: @.***>

Hi Zach,
I manually reviewed the local changes you made to the cast_type pull request and I see no issues with your changes.
Do you still need me to update my pull request?
Richard
From: Richard Herveille @.>
Date: Tuesday, 3 September 2024 at 09:17
To: YosysHQ/yosys @.>, YosysHQ/yosys @.>
Cc: Author @.>, Richard Herveille @.***>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
I am not using a personal account. Github doesn’t allows "Maintainers are allowed to edit this pull request." To non-personal accounts.
I will need to find time to pull in your changes. But time is currently extremely limited.
Richard
From: Zachary Snow @.>
Date: Sunday, 1 September 2024 at 06:50
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>, Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
To short-circuit another round of reviews and edits, I've made some edits locally. I'd like to add them to this PR and have you take a look before I merge them. Can you either enable "Maintainers are allowed to edit this pull request." or pull in my changes at https://github.com/zachjs/yosys/tree/cast_type?
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2323348635, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCNNONLUYF3KDNXZOSLZUMLR5AVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGM2DQNRTGU.
You are receiving this because you authored the thread.Message ID: @.***>

Yes, could you please apply or cherry-pick my patch?

Only the edits to my files, right?
Richard
From: Zachary Snow @.>
Date: Monday, 23 September 2024 at 17:25
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>, Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
Yes, could you please apply or cherry-pick my patch?
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2369845990, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCN3OKFDVL3DCCPSWEDZYCWQ3AVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRZHA2DKOJZGA.
You are receiving this because you authored the thread.Message ID: @.***>

Yes. To be clear, I'm imagining you'll run something like:
git fetch https://github.com/zachjs/yosys.git cast_type
git cherry-pick FETCH_HEAD
(build, test, etc.)
git push

Done, the code builds and all tests pass
Richard
From: Zachary Snow @.>
Date: Monday, 23 September 2024 at 20:23
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>, Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
Yes. To be clear, I'm imagining you'll run something like:
git fetch https://github.com/zachjs/yosys.git cast_type
git cherry-pick FETCH_HEAD
(build, test, etc.)
git push
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2370033311, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCMCMLMC4AM2FVO3NJLZYDLJDAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZQGAZTGMZRGE.
You are receiving this because you authored the thread.Message ID: @.***>

[like]  Richard Herveille reacted to your message:

From: Zachary Snow @.>
Sent: Sunday, September 29, 2024 9:03:22 PM
To: YosysHQ/yosys @.>
Cc: Richard Herveille @.>; Author @.>
Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)
Merged #4284https://github.com/YosysHQ/yosys/pull/4284 into main.
—
Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#event-14446656291, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCP5JZQ4SSWV6HEHNFTZZBTJVAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGQ2DMNRVGYZDSMI.
You are receiving this because you authored the thread.Message ID: @.***>