proposal: new cryptographic assembly policy

The crypto packages are unfortunately rich in assembly implementations, which are especially hard to review and maintain due to the diversity of architectures, and to the complexity of the optimizations.
Our current assembly policy at golang.org/wiki/AssemblyPolicy has failed to make reviews manageable with the resources the security team currently has. https://go-review.googlesource.com/q/hashtag:crypto-assembly
The result is suboptimal for everyone: implementers have to follow more rules, but reviewers still can’t effectively review their CLs, and no one is happy.
I am proposing a new, much stricter policy. This acknowledges the reality that assembly reviews are currently not moving at an acceptable pace, and shifts more of the load on the implementers, but with a promise that their work won’t go wasted in the review queue. It should also progressively increase the maintainability, reliability and security of the assembly codebases, as well as surface improvement areas for the compiler so that the assembly can be eventually removed.
This policy would apply to all packages in crypto/... and golang.org/x/crypto/.... Due to its use in cryptographic packages, and to the fact that it's partially maintained by the security team, this policy would also extend to math/big.

We prefer portable Go, not assembly. Code in assembly means (N packages * M architectures) to maintain, rather than just N packages.
Minimize use of assembly. We'd rather have a small amount of assembly for a 50% speedup rather than twice as much assembly for a 55% speedup. Explain the decision to place the assembly/Go boundary where it is in the commit message, and support it with benchmarks.
Use higher level programs to generate assembly, either standalone Go programs or go get-able programs, like avo. Output of other reproducible processes (like formally verified code generators) will also be considered. Discuss the implementation strategy on the issue tracker in advance.
Use small, testable units (25–75 lines) called from higher-level logic written in Go. If using small, testable units called from logic written in Go is too slow, use small, testable assembly functions with Go-compatible wrappers, so that Go tests can still test the individual units.
Any assembly function needs a reference Go implementation, that’s tested and fuzzed side-by-side with the assembly. Follow golang.org/wiki/TargetSpecific for structure and testing practices.
Document in the Go code why the implementation requires assembly (specific performance benefit, access to instructions, etc), so we can reevaluate as the compiler improves.

/cc a number of crypto assembly implementers: @mundaym @laboger @mengzhuo @ceseo @crvv @TocarIP @aead @vkrasnov @ncw @gtank @bmakam-qdt

In general the policy seems good to me, though I do feel that if a contributor is following the current assembly policy that they would already be meeting the bar of the new one. Maybe we need to expand the advice even more, I'm not sure.
One thing that this proposal doesn't address is the fairly common practice of taking assembly from another project (e.g. OpenSSL) and porting it to Go assembly. I see this in reviews every so often. One concrete thing that I think we should encourage people to do is to document the repositories and, importantly, the exact version control revisions that their code is based on so that it is easier to check for upstream changes. Having said that, when reviewing such code do you think that it is better to encourage keeping the code close to the original so that it is easier to update with upstream changes (assuming anyone ever bothers to check for them) or to modify it more heavily to fit with the policy? Sometimes contributors don't fully understand the implementation or algorithm they are porting and so it will be very hard for them to rework it to fit with the policy. How firm should we be in that sort of situation, assuming it is otherwise well tested?
One other point I'd like to make is that I don't think we should mandate the use of code generation. It should only be used when it would increase maintainability. Short hand-written assembly files can be preferable in some situations.

Yes, I agree, I switched from assembly in go-ristretto to using plain Go for Go 1.13 as it has bits.Mul64.  Performance is a bit worse, but an acceptable trade-off for the increased maintainability.  It would be nice to have more "intrinsics" support in Go, like Rust's u32x4, etc. Otherwise I don't see how to reach acceptable performance for hashes.

@bwesterb please do file issues when you encounter specific intrinsics that are important for eliminating assembly. There’s uncertainty around vector intrinsics, but I think there’s openness to new scalar intrinsics.

I seem to remember a discussion when we wrote https://github.com/golang/go/wiki/AssemblyPolicy (only 18 months ago). Does anyone know if there's a link somewhere to that discussion?

In general the policy seems good to me, though I do feel that if a contributor is following the current assembly policy then they would already be meeting the bar of the new one. Maybe we need to expand the advice even more, I'm not sure.

Hmm, the major differences should be a stronger preference for code generation, and the requirement for a Go reference implementation that matches small assembly functions 1:1. For example, there are generic implementations that do 32-bit limbs while the assembly does 64-bit limbs, and that would not be acceptable anymore, or assembly where the only entry point is ScalarMult, which would have to be broken up in a dozen or so functions.

One thing that this proposal doesn't address is the fairly common practice of taking assembly from another project (e.g. OpenSSL) and porting it to Go assembly. I see this in reviews every so often. One concrete thing that I think we should encourage people to do is to document the repositories and, importantly, the exact version control revisions that their code is based on so that it is easier to check for upstream changes. Having said that, when reviewing such code do you think that it is better to encourage keeping the code close to the original so that it is easier to update with upstream changes (assuming anyone ever bothers to check for them) or to modify it more heavily to fit with the policy? Sometimes contributors don't fully understand the implementation or algorithm they are porting and so it will be very hard for them to rework it to fit with the policy. Or they may just be reluctant to change the code too much. How firm should we be in that sort of situation, assuming it is otherwise well tested?

I should mention that by default, this is not ok license-wise. There's a path to bring in assembly that was dual-licensed as part of cryptogams, and that's what was taken a couple times.
I think we tried the "keep the port close to the source" route, and it does not meet the standards of maintainability we'd like to have, also because the source is often in perlasm or something like that, and the calling conventions are different, and Go has reserved registers, etc.
I'd like us to try developing a small and curated set of assembly implementations rather than bring in large amounts from other projects with different priorities.

One other point I'd like to make is that I don't think we should mandate the use of code generation. It should only be used when it would increase maintainability. Short hand-written assembly files can be preferable in some situations.

This is particularly experimental, and I'm willing to reconsider after we've seen some results, but I'd like to try it out, as I think it might lead to much better readability and portability. Maybe it will even force the creation of more tools like avo that can abstract some of the useless manual work.

Should the new assembly policy make any statement about when assembler code eventually gets removed or is very unlikely to get merged?
Probably it's not worth further maintaining M asm implementations of a "broken" cryptographic primitive. One example, I remember, has been the crypto/rc4 package. See: #25417 and https://go-review.googlesource.com/c/go/+/130397/
Similarly, we may not want to maintain more asm code for "outdated" primitives - i.e. SHA-1.
(Yeah, technically SHA-1 is broken as well - but it's still commonly used b/c legacy, git, ...). For such primitives it seems unlikely that new asm code is worth the maintenance cost.
However, both aspects are quite specific for cryptographic implementations. So they may be to specific for a general assembly policy.

Use higher level programs to generate assembly, either standalone Go programs or go get-able programs, like avo.

Can we clarify how "high level programs to generate assembly" make the code more secure? If anything, I would think they require more scrutiny. The author of a program that calls a third-party program to generate assembly has no idea why the program generated it the way it did. They need to understand their implementation in their high level language as well as review and understand the low-level assembly generated by the third-party program. It is a mistake to delegate trust to a third party code generator to generate assembly for a unique cryptographic primitive.

Output of other reproducible processes (like formally verified code generators) will also be considered.
Which processes? What is the context of this statement? Are we talking about guarantees that the instructions aren't corrupting memory or verifying that the implementation itself is doing the right thing for all possible inputs. The latter seems impossible, and the intent of formal verification should be clearly stated in the proposal to reduce confusion.

Hi @FiloSottile, FWIW I like the general philosophy here around active experimentation to see if a new approach yields better results, and it sounds like the concept is to observe and refine based on how the experiment goes.
A few questions on the fuzzing piece:

Would it make sense to require hooking up to oss-fuzz or similar for on-going fuzzing?
Would it help to include somewhere some sample template fuzzing functions, including side-by-side fuzzing, or perhaps instead include some pointers to some good concrete examples? (Maybe this would not be part of the policy itself, but perhaps on an auxiliary wiki page or similar).
Would it make sense to recommend or possibly require fuzzing with > 1 engine (such as native dvyukov/go-fuzz as well as libFuzzer) given different engines often surface different bugs? In other words, the policy wouldn't necessarily need to say you must use engine X and Y, but could say the count of engines used should be > 1.

Can we clarify how "high level programs to generate assembly" make the code more secure? If anything, I would think they require more scrutiny. The author of a program that calls a third-party program to generate assembly has no idea why the program generated it the way it did. They need to understand their implementation in their high level language as well as review and understand the low-level assembly generated by the third-party program. It is a mistake to delegate trust to a third party code generator to generate assembly for a unique cryptographic primitive.

Firstly a disclaimer: I wrote avo. I'm pleased to see it mentioned in this proposal as one of many tools available to help reduce complxeity in high performance crypto assembly. That said, I'm not naive enough to think it's always the right tool for the job!
The claim is that code generation makes code easier to write, review and maintain, ultimately making bugs less likely. It allows us to assign meaningful names to variables, and use control structures to handle the repetitive block structures that are common in cryptographic primitives (avo SHA-1 example).  I would claim that concise code generators written this way are more likely to be reviewable than thousands of lines of assembly (see p256 and AES-GCM).
I would also point out that this isn't a novel idea, in fact code generation is already used extensively in crypto assembly, specifially via the preprocessor macros.
Preprocessor being used for "register allocation" in AES-GCM:
https://github.com/golang/go/blob/3eab754cd061bf90ee7b540546bc0863f3ad1d85/src/crypto/aes/gcm_amd64.s#L14-L21
Here's a poor-man's "function" defined with the preprocessor:
https://github.com/golang/go/blob/3eab754cd061bf90ee7b540546bc0863f3ad1d85/src/crypto/aes/gcm_amd64.s#L264-L275
Likewise in P-256:
https://github.com/golang/go/blob/3eab754cd061bf90ee7b540546bc0863f3ad1d85/src/crypto/elliptic/p256_asm_amd64.s#L18-L25
https://github.com/golang/go/blob/3eab754cd061bf90ee7b540546bc0863f3ad1d85/src/crypto/elliptic/p256_asm_amd64.s#L1664-L1683
Again in blake2b
https://github.com/golang/crypto/blob/86ce3cb696783b739e41e834e2eead3e1b4aa3fb/blake2b/blake2b_amd64.s#L33-L40
I could go on. My point is that code generation is already being used by Go crypto in some sense, it's just the pre-processor is a particularly awful form of it (in my opinion). And Go isn't alone here: OpenSSL's perlasm isn't something we'd like to copy, but it's further evidence of code generation in this space. Likewise qhasm in SUPERCOP/NaCL and other projects. I think there's a case to be made that the proposed assembly policy isn't saying that much new, it's just encouraging people to use better tools than the Go assembly preprocessor.
Your point that it's also necessary to review the code generator itself is completely fair and correct. I'd make a few points in response to this, some general, some defending avo in particular.

You don't have to use a third-party package at all. It can simply be a make_asm.go program that prints assembly output. Perhaps something like this meow hash assembly generator.  It can be even simpler than this as well. There's nothing prescriptive about this policy, only bring in a third-party package if you think it's warranted.
avo isn't that complicated. I'm proud of the project, but I wouldn't never claim it was rocket science. I have deliberately avoided features that would drift into "compiler" territory.  It contains a Go wrapper around every x86 instruction as well as a register allocator. Most of the complexity lies in the register allocation (and associated passes), and that logic ultimately isn't that much code. Apart from that it's a cleaner way to print assembly isntructions, and allows you to use Go functions and control structures to structure your code generator.
avo is as complex as you want it to be. If you're suspicious of the register allocator and associated complexity, you don't have to use it.  avo supports just using physical registers directly.
When done well, the assembly is reviewable too. I wouldn't claim this was always possible, but tools like avo produce assembly output (they are not assemblers). You can insert comments in the assembly output as well, and it'll format it nicely, like asmfmt.  When this is done carefully, the output actually looks quite nice. If you can achieve this you are in a strictly better situation than you were before: you can still review the raw assembly but you have the code generator alongside it too.

Which processes? What is the context of this statement? Are we talking about guarantees that the instructions aren't corrupting memory or verifying that the implementation itself is doing the right thing for all possible inputs. The latter seems impossible, and the intent of formal verification should be clearly stated in the proposal to reduce confusion.

I can't speak for @FiloSottile, but based on a conversation we had at Real World Crypto I have some context. It seems that after the HACS workshop there is some chance that the fiat-crypto team will add support for Go output. If so this could be a great outcome for the Go community, and I assume the intention was for the assembly policy to encompass auto-generated code from formally verified tooling.
@FiloSottile I appreciate you putting this together and I'm in broad agreement.  I'm glad to see the emphasis that this is an experiment -- we can learn as we go. I wonder if it might be a good idea for us to concretize this in some specific cases, to see how it might play out. Perhaps we could pick one or more of the critical primitives and ask ourselves how we would have implemented it under the proposed policy (for example P-256, AES-GCM, curve/ed25519). By the way I'm not suggesting we write any code, just that we sketch out what a policy-compliant implementation would look like.

I am proposing a new, much stricter policy. This acknowledges the reality that assembly reviews are currently not moving at an acceptable pace, and shifts more of the load on the implementers, but with a promise that their work won’t go wasted in the review queue.

I've contributed several ARM crypto implementations. It is work I enjoy and am suited to having written 10s of thousands of lines of ARM assembler in the past. However spending weeks writing assembly code to not have it merged really sucks!
I think this is an excellent initiative. It will require some restraint on the part of the assembly authors who naturally want to go for as fast as possible.
I like the idea of code generation. I've used the preprocessor extensively along with register name definitions which makes life easier. The preprocessor is a poor second to a good macro assembler though.
Alas avo is x86 only at the moment isn't it? I'm not sure there are any ARM tools at the moment (I'd love to be corrected if wrong).

As for code generation, There are examples for comparison.
https://go-review.googlesource.com/c/go/+/51670
https://go-review.googlesource.com/c/go/+/136896
The code in CL 51670 is hand-written. The code has the exact same structure with the s390x implementation.
The .s file has 124 lines. I think all lines below line 71 is trivial(they are copied from $GOROOT/src/crypto/aes/asm_amd64.s).
There are some repetitions in the first 70 lines. If a reviewer reads 50 lines in the .s file, I think the code can be understood very well.
The code generator in CL 136896 has 298 lines. It is clear that I need more time to read the 298 lines of Go than those 124 lines of assembly. Besides, there are another 740 lines of assembly code to review.
If I am the reviewer, I need much more time to review CL 136896.
Although the code in CL 51670 is written by me, I still think the point is reasonable for others.
Code generation can be helpful in some situations, but it doesn't fit all situations.

@crvv According to my measurements these two CLs have substantially different performance. See the following gist for the methodology, and please let me know if I got something wrong.
https://gist.github.com/mmcloughlin/70598689ad2da5c6aadf19e30b6642a1
Here's the benchcmp output between the two CLs:
benchmark                   old ns/op     new ns/op     delta
BenchmarkAESGCMSeal1K-8     556           558           +0.36%
BenchmarkAESGCMOpen1K-8     530           534           +0.75%
BenchmarkAESGCMSign8K-8     2049          2045          -0.20%
BenchmarkAESGCMSeal8K-8     3366          3362          -0.12%
BenchmarkAESGCMOpen8K-8     3284          3289          +0.15%
BenchmarkAESCTR1K-8         671           376           -43.96%
BenchmarkAESCTR8K-8         4826          2294          -52.47%

benchmark                   old MB/s     new MB/s     speedup
BenchmarkAESGCMSeal1K-8     1840.43      1834.91      1.00x
BenchmarkAESGCMOpen1K-8     1929.99      1917.47      0.99x
BenchmarkAESGCMSign8K-8     3997.98      4005.84      1.00x
BenchmarkAESGCMSeal8K-8     2433.57      2436.21      1.00x
BenchmarkAESGCMOpen8K-8     2494.19      2490.21      1.00x
BenchmarkAESCTR1K-8         1516.47      2704.17      1.78x
BenchmarkAESCTR8K-8         1696.40      3568.81      2.10x

Note I've included AES-GCM benchmarks partly to show the benchmarks ran in similar environments, but also because it should be possible to write an AES-CTR mode implementation with similar performance to AES-GCM, if not better.
I believe this experiment shows that CL 136896 is approx 2x faster than CL 51670. Therefore this is a classic trade-off. CL 136896 is unquestionably more complicated than 51670, but one could argue the complexity is justified for the speedup.
CL 136896 uses some of the techniques employed in the GCM implementation, producing performance fairly close to GCM for the 8K benchmark. But I would argue that the code generator used in the CL makes it easier to understand than gcm_amd64.s (1286 hand-written lines).

There is no doubt that the complex one is faster.
The point is the code generator is not always helpful.
If you write the simple algorithm in CL 51670, I doubt you will use the code generator.
The benefit is small.
As for the speed difference between the two CLs, it depends on the CPU architecture.
CL 136796 is 10%-100% faster than CL 51670.
By the way, on my computers, OpenSSL 1.1.1d is 20%-80% faster than CL 136896.

@crvv wrote:

The point is the code generator is not always helpful.

I'd agree in this case. I think you could do everything the code generator does with preprocessor macros in the .s file itself and it would be clear what is going on rather than having to work out how the code generator works then puzzling through how what the code generator did matches the assembler code generated.
I personally think the solution to this is to make the macro system in the go assembler more powerful and then mandate that all assembler code is written with that, rather than third party or ad-hoc code generation tools.

There is no doubt that the complex one is faster.
The point is the code generator is not always helpful.
If you write the simple algorithm in CL 51670, I doubt you will use the code generator.
The benefit is small.

@crvv Yes, fair point and I probably agree that CL 51670 wouldn't benefit from code generation.
I was reacting to your previous comment in which it seemed to me you compared the two CLs as if the only difference was one uses code generation and the other doesn't. One is up to 2x faster than the other, that needs to be mentioned in any comparison.

By the way, on my computers, OpenSSL 1.1.1d is 20%-80% faster than CL 136896.

Yes, CL 136796 isn't optimal either.

To clarify, the goal is to present the assembly in the simplest, most reviewable form. Sometimes it is small and that's a direct .s file. Sometimes not, and a generator is better. And yes, one can write difficult-to-understand generators - don't do that. Strive to find the simplest way to express what you need to express. That's true for all code.
I think everyone basically agrees that CL 51670 doesn't need a generator, although I think it's close to the upper limit for that category. And I think basically everyone agrees that CL 136896's generator is much better than the 700 lines of assembly it generates. The question of whether a particular optimization is worth the added complexity is almost always going to be context-dependent. (In that specific case, a factor of two was deemed worthwhile. We're not here to second-guess that.)

From reading the discussion, it sounds like people are generally in favor of the policy @FiloSottile put in the comment atop this issue. Do I have that right? Is anyone opposed to this policy?
(There was some discussion about tradeoffs in the specific AES-CTR case, but the policy is not about settling every possible tradeoff.)

Is anyone opposed to this policy?

As far as I can tell from the discussion, there is pushback against the phrasing "Use higher level programs to generate assembly", in that it appears to suggest code generation is required. As you say it seems there is consensus here that for small enough programs code generation may be counterproductive.
Perhaps a slight softening of that language would be good? It would be nice if we could be more specific about when code generation is required, but it seems quite difficult to strictly define.

I agree with most of what's written in principle. Use asm only when there is no other way. Write it as clear as possible, document well, test well. Good rules to follow for any asm not just crypto.
I would just like to point out that much of the policy is written with amd64 in mind. There is no avo for others. The performance of generated code for Go is more likely to be worse than asm if not amd64. Writing small functions could result in more call overhead if the functions can't be inlined.

Thanks everyone for the input and the discussion.
Re: code generation, this is definitely an experiment, and I agree with Russ that there can be more and less complex generators, and they should be weighted against the complexity they save. They can be as simple as some Go functions named after a specific operation (say, field addition) that take parametrized registries and print assembly, so they can be reused and looped. How about the following language? I think "Discuss the implementation strategy on the issue tracker in advance." gives us space to assess case by case as we go forward.

Use higher level programs to generate non-trivial amounts of assembly, either standalone Go programs or go get-able programs, like avo. Output of other reproducible processes (like formally verified code generators) will also be considered. Discuss the implementation strategy on the issue tracker in advance.

I would just like to point out that much of the policy is written with amd64 in mind. There is no avo for others. The performance of generated code for Go is more likely to be worse than asm if not amd64. Writing small functions could result in more call overhead if the functions can't be inlined.

That's definitely not the intention, as that would only solve a fraction of the maintenance problem, so I would love to hear more.
Asm is going to be faster than compiler-generated amd64 as well, even if the gap might be smaller. The requirement to highlight what the compiler needs to do to replace the asm is meant to fight that gap over time.
When you say small functions do you mean asm or Go?  The requirement is specifically for asm functions, and I realize sometimes using the Go ABI is too expensive, so it's ok to make simple jumpable units with test adapters.

Inlining small assembly functions probably falls under https://github.com/golang/go/issues/17373

@filosottile, could you please post in a new comment the exact policy that we are now discussing accepting or rejecting. I see an edit a few comments back and just want to make sure everyone agrees how it applies.
@dgryski, note that we won't be inlining arbitrary small assembly functions any time soon. Too much subtlety for the compiler to take on. The path we chose instead was good intrinsified APIs.

Hello, if the above policy is passed, does the golang community need to reconstruct some existing assembly implementations with more than 100 lines? Such as splitting into smaller and more assembly or golang functions. Compared with the current assembly implementation, this way may lead to performance degradation. Will this be accepted by the golang community?

Hello, if the above policy is passed, does the golang community need to reconstruct some existing assembly implementations with more than 100 lines?

Yes, eventually we do want to adapt current code to the policy, although that's lower priority than draining the current review queue. Like with new code, performance is not the ultimate goal, but I'd like to think that as we gain experience with the policy, the degradation will be small or null.

@FiloSottile, could you please post in a new comment the exact policy that we are now discussing accepting or rejecting. I see an edit a few comments back and just want to make sure everyone agrees how it applies.

Here's the full current text. I also clarified functions vs units.

We prefer portable Go, not assembly. Code in assembly means (N packages * M architectures) to maintain, rather than just N packages.
Minimize use of assembly. We'd rather have a small amount of assembly for a 50% speedup rather than twice as much assembly for a 55% speedup. Explain the decision to place the assembly/Go boundary where it is in the commit message, and support it with benchmarks.
Use higher level programs to generate non-trivial amounts of assembly, either standalone Go programs or go get-able programs, like avo. Output of other reproducible processes (like formally verified code generators) will also be considered. Discuss the implementation strategy on the issue tracker in advance.
Use small, testable units (25–75 lines) called from higher-level logic written in Go. If using small, testable functions called from logic written in Go is too slow, use small, testable assembly units with Go-compatible wrappers, so that Go tests can still test the individual units.
Any assembly function needs a reference Go implementation, that’s tested and fuzzed side-by-side with the assembly. Follow golang.org/wiki/TargetSpecific for structure and testing practices.
Document in the Go code why the implementation requires assembly (specific performance benefit, access to instructions, etc), so we can reevaluate as the compiler improves.

Does anyone object to the policy in the preceding comment by @FiloSottile?

I think the policy looks good.
It might be worth mentioning here that the output of the assembly generators needs to be checked in and have a comment at the start like // Code generated by programName. DO NOT EDIT

Use higher level programs to generate non-trivial amounts of assembly, either standalone Go programs or go get-able programs, like avo. Output of other reproducible processes (like formally verified code generators) will also be considered. Discuss the implementation strategy on the issue tracker in advance.

Use higher level programs to generate non-trivial amounts of assembly, either standalone Go programs or go get-able programs, like avo. Output of other reproducible processes (like formally verified code generators) will also be considered. Discuss the implementation strategy on the issue tracker in advance.

How much assembly quantifies a non-trivial amount?

Worth noting that @ncw's comment is in the go help generate output but would be good to repeat in this policy.
@as, non-trivial will always be in the eye of the reviewer. Most of the time it's easy to decide. Discussion on the issue tracker in advance will help.

Based on the discussion above, this seems like a likely accept.

No change in consensus, so accepted.

This is now golang.org/wiki/AssemblyPolicy and will apply to the Go 1.16 cycle.


chore: migrate most jsonpb usages to proto v2 API

Thank you for doing this! Can you check if this fixes #4288 ?


Content not associated with a region is not pruned from ISD before deciding if it's empty

Right now, the imsc-hrm-tests dur003-fail passes when it should fail. See also w3c/imsc-hrm-tests#9.
It used to fail in imscHRM because the previous ISD beginning at 2.5s was considered non-empty, and therefore made the available time for the following ISD beginning at 3s be 0.5s. However, that previous ISD only contained content not associated with any region, and should therefore have been pruned. If it had been pruned, then the ISD should have been considered empty, and therefore the ISD beginning at 3s would have had a full 1s to render, and that older variant of the test should have passed instead of failing.

However, that previous ISD only contained content not associated with any region, and should therefore have been pruned.

Even though it is pruned, so it still generates an ISD, which, because the region background is showAlways, triggers a background clear and region paint.


UI fixes and cleanup

Checklist

[ ] Closing issues: #issue
[X] Mark this if you consider it ready to merge
[ ] I've added tests (optional)
[ ] I wrote some documentation

Description
Fixing modals, focus, etc. Cleaned up to use the same new auto.py

You are on fire 🔥


It is not possible to launch tests simultaneously

All tests execute one by one. Is it possible to execute TestGroup simultaneously?

It isn't possible as we are relying on Nunit to run the tests.

Closing issue, this is something out of our hands. And for best (and most reliable) results, you don't want tests to run at the same time.


Configuration update : Splashscreen with company logo

see @HugoNDO  for design

use tagline from configuration jwt and logo url

https://www.figma.com/file/gNQ9POVTHhxZQbzRQl6Qks/Altme?type=design&node-id=7390%3A20636&mode=design&t=8EiQ6c4q9eKefFks-1


Enhance Document Conversion Service with FastAPI Export Endpoints

This pull request represents a significant enhancement to the Document Conversion Service, incorporating a robust set of FastAPI endpoints for exporting data to Office formats. The newly developed API services are rigorously tested and support conversions from Markdown to both DOCX and PPTX files, expanding the application's functionality.
Key achievements of the day include:

Development and meticulous testing of export API services using FastAPI framework, ensuring reliable markdown to DOCX and PPTX file conversions.
Crafting and execution of unit tests for the new API endpoints, confirming both functionality and the integrity of responses.
Reorganization and restructuring of the project repository to segregate Next.js and FastAPI applications, streamlining the development and deployment processes within the feature branch feature/104-docker.

These updates underpin the pull request by providing new, efficient ways for users to export their query results directly into user-friendly formats, thereby enhancing the overall utility and performance of the Document Conversion Service. The integration of these services into the main branch represents a leap forward in the usability of the platform, offering an end-to-end solution from querying data to obtaining polished, presentation-ready documents.

closes #104


fix conflicts after merging new save formats using .npz files

This PR aims at fixing conflicts after merging the new architecture of the repo.
It also syncs the jupyter notebook and the README file.

@danielriosgarza I think we can now remove the gapfill_function.py script, would you be ok with that?


Add CSSOM polyfill

Oh nice, that would fix #21, and remove the need to load css-typed-om ourselves :-)
Do think the PR is lagging one commit behind against master though, specifically the changes done in #28 (which replaced startScrollOffset/endScrollOffset with scrollOffsets)

Abandoning this pull request in favor of pull #33.

Thanks for pointing out the conflict Bramus.


Provide a DogStatsD API in the sidecar

What does this PR do?
Add a DogStatsD client in the sidecar, and expose an API to send metrics through it.
Motivation
Is part of the PHP tasks for Q2. It adds new possibilities for the integration team.
Additional Notes
The underlying StatsD client used by this PR is Cadence.
It is already used by some teams at Datadog, even if does not complies exactly to other DogStatsD implementations like the one in PHP or Go
For example, it lacks support of the "sample rate", or requires an integer for the Set metric, while Go and PHP can accept a string.
But I think there is nothing critical that blocks us.
How to test the change?
Describe here in detail how the change can be validated.
For Reviewers

[ ] If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
[ ] This PR doesn't touch any of that.

Codecov Report
Attention: Patch coverage is 50.67114% with 147 lines in your changes are missing coverage. Please review.

Project coverage is 65.19%.  Consider uploading reports for the commit 567bded to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
- Coverage   65.36%   65.19%   -0.18%
==========================================
  Files         184      185       +1
  Lines       22503    22798     +295
==========================================
+ Hits        14710    14864     +154
- Misses       7793     7934     +141

Components
Coverage Δ

crashtracker
20.31% <ø> (ø)

data-pipeline
51.45% <ø> (ø)

data-pipeline-ffi
0.00% <ø> (ø)

ddcommon
81.52% <ø> (+0.29%)
:arrow_up:

ddcommon-ffi
74.93% <ø> (ø)

ddtelemetry
53.72% <ø> (ø)

ipc
81.27% <ø> (ø)

profiling
76.89% <ø> (ø)

profiling-ffi
60.05% <ø> (ø)

serverless
0.00% <ø> (ø)

sidecar
31.39% <50.67%> (+2.02%)
:arrow_up:

sidecar-ffi
0.00% <0.00%> (ø)

spawn-worker
54.98% <ø> (ø)

trace-mini-agent
69.12% <ø> (ø)

trace-normalization
97.79% <ø> (ø)

trace-obfuscation
95.74% <ø> (ø)

trace-protobuf
25.64% <ø> (ø)

trace-utils
68.85% <ø> (ø)


too big indent for files

files with seti-icons look like children of the last folder

I suggest to change margins of .icon::before a little bit:
atom-workspace .entries.list-tree .file .icon::before {
  margin-left: -4px;
  margin-right: 3px;
}

@erroid would you want to submit a PR for this change? Also, it looks like this is how it is in jesseweed/seti-ui so you might want to open an issue there as well. I think it looks better lined up like your 2nd screenshot.

alright, created here: https://github.com/jesseweed/seti-ui/issues/360


[XLA:GPU][ROCm] Adding ROCm parallel logic for cudnn_conv_algorithm_picker

This PR is the phase2 follow-up of PR #30884. This PR creates renames from cudnn_conv_algorithm_picker to gpu_conv_algorithm_picker. In the gpu_conv_algorithm_picker, I added

PickBestAlgorithmNoCacheROCm()
PickBestAlgorithmNoCacheCuda()

to make the pass available to both Cuda and ROCm by branching from runtime.
Please review the PR follows the sequence of commits: First commit does only renaming. Second commit adds ROCm parallel functionality.

@cheshire @whchung

@jerryyin Could you please resolve the conflicts? Thanks!

@gbaned Thanks for reminding. Since this PR has a dependent renaming PR, I'd prefer to rebase this one al-together when the dependent PR is out and merged.

@cheshire Gentle ping

This currently fails on some internal tests in CUDA configuration. I can look into that, but it might be faster to try to minimize the diff for the CUDA logic (which should be empty, right?)
Currently, I see two differences:

params.algorithm gets passed two extra arguments
options.algo_override.has_value() hack

I think we have discussed those before. Would it be hard to remove these, or make them CUDA-specific?

Can I help this land?

@cheshire Sure, that'll be much appreciated. I wrapped the ROCm specific call within the kROCm platform condition so it shouldn't affect CUDA now. Let me know if you run into any issues into internal tests. Thanks

Thank you for the help @yifeif


Make website objective clear and attractive user interface

What Issues I have faced while using the website:

At first time, I can't get the objective of website. Confusions like The website is for CSE only? Or engineering materials only?
Need to clear the objective, The website is For all the fields of Engineering and For Engineering only or for other courses also?
Website is not responsive.
In Navbar, Nav Link "Notes" should have a dropdown like Question papers.
The Home page is having so much space, making the website unattractive.

What I can Do:

As the experienced Front end developer, I can help you with increasing User Interface attractiveness and making the Website responsive.
Modify the design of the website such that it's objective becomes clear to the students.

Additional Feature:

Allow students to upload their handwritten notes.
Students should be allowed to provide feedback also.
Provide registration option for more customized search of resources.

Issue of responsiveness:

Issue of color combination:

If you find these changes critical for your project, please provide necessary assignment and badges.
Thank you!

i want to imporive the navigation bar in mobile responsive and alignment of the component are not properly :

@17arindam  Could you please assign this home page design enhancement task to me and label it for contribution? I’m excited to start working on it. Thank you!


Cannot create whatsaround needs

It seems that what's around broke completely recently, maybe due to a change in the OSM api?

Cannot be reproduced consistently..


Stuck on ansible [consul | restart consul ] after adding more worker nodes.

If I decide for given terraform cluster to add more worker nodes  and run ansible playbook again after adding more node....it will get  stuck on consul restart...with not being able to proceed nor timeout will kick in.
Below in screenshot I am re-running terrform.yml again to get new nodes worker002/03 in the cluster.
I tried this several time. Same issue every time.
TASK: [consul | deploy acl bootstrap] *****************************************
ok: [mi-edge-01]
ok: [mi-worker-001]
ok: [mi-control-02]
ok: [mi-control-03]
ok: [mi-control-01]
changed: [mi-worker-003]
changed: [mi-worker-002]

TASK: [consul | upsert agent acl] *********************************************
changed: [mi-edge-01]

TASK: [consul | create agent_acl consul config] *******************************
ok: [mi-edge-01]
ok: [mi-control-03]
ok: [mi-control-02]
ok: [mi-worker-001]
ok: [mi-control-01]
changed: [mi-worker-003]
changed: [mi-worker-002]

NOTIFIED: [consul | restart consul] *******************************************

If I do fresh installation , works great.
I am testing this on Cisco Cloud ( Openstack)

Also to mention , I am tunneling trough bastion hosts with ansible to reach hosts in openstack private network . I did noticed some weird behavior when running for example upgrade packages yml , it would not recognize that machine came back online after reboot.
I had to modify upgrade-packages.yml to get past trough never being able to recognize if hosts came back up.
  - name: wait for host boot
    local_action:
      module: wait_for
      host: "{{ ansible_ssh_host }}"
      #port: 22
      delay: "{{ boot_wait | default(100) }}"
      timeout: 120
      state: started
    when: reboot.changed

I commented port and changed boot wait default to 100 sec.

this should be fixed by #1070. please re-open if there are still issues


You cannot just choose any resource name

glb_master_exists=`echo "$resources" | grep -A2 $glb_master_resource | egrep -c 'Master[^\/]'`

The code uses this a lot...  glb_master_resource... grep..
so if you have p_mysql and p_mysql_2, then well... problems again...

Fixed by commit 6ad4eef6610eff3354f89433512c3c152f3d260d


Add Doc changes for GPU Recommendations via List Recommendations API

Description
This PR adds doc changes for GPU Recommendations via List Recommendations API
This PR is build on top of the PR's which adds GPU mechanism to make sure they are merged before adding the docs.
Fixes #1312
Type of change

[ ] Bug fix
[ ] New feature
[x] Docs update
[ ] Breaking change (What changes might users need to make in their application due to this PR?)
[ ] Requires DB changes

How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.

[ ] New Test X
[ ] Functional testsuite

Test Configuration

Kubernetes clusters tested on:

Checklist :dart:

[ ] Followed coding guidelines
[ ] Comments added
[ ] Dependent changes merged
[ ] Documentation updated
[ ] Tests added or updated

Additional information
Include any additional information such as links, test results, screenshots here

list_recommendations.json
generate_recomemndation.json
The content of the docs is based on the ListRecommendations API and GenerateRecommendation API outputs which are attached to this comment.


 page title don't show

when F5 refresh page can show title true.

Can you provide more information about your code ?
thanks.

index page head:
  head: {
    title: 'Buy FIFA Coins, NBA2K MT, Madden Mobile Coins and NBA Live Coins Online',
    titleTemplate: '%s - LOLGA',
    meta: [
      { hid: 'keywords', name: 'keywords', content: 'aaafifa 17 coins, fifa 17 points, cheap NBA 2K17 coins, buy madden 17 coins, NHL 17 coins, cheap fifa coins, buy wow gold, runescape gold, Nhl coins, madden coins, gamecard, cd-key' },
      { hid: 'description', name: 'description', content: 'Lolga™ is absolutely a right place for you to buy Fifa Coin(Aka Fut Coins), cheap fifa 17 points, Wow gold. We Provide with Really cheap price and 24*7 Online Service, Fast Delivery all exist here. What are you waiting for? Trust US!' }
    ]
  },
  components: {
    Carousel
  }

then is checkout page:
  head: {
    title: 'One Step Checkout'
  }

it's look no problem . and working on me .
Any error on your browser console or any where?

chrome console don't hava any error.
thanks for you

maybe something error with other reason ...
this is your provide code test on glitch test
https://mica-sociology.glitch.me/
source

i see.
let me check code again :blush:

I found the reason.
because my checkout page import element
when delete element table plugin title is true

umm..  I guess have some issue with SSR you need fix .
You should close this issue , find the problem from closed issue ( maybe )
thanks.

thanks @ausir0726


url-polyfill is loaded in Safari when not required

Seems to be because Safari escapes the pathname set by url-polyfill.js#L173, causing the comparison (u.href === 'http://a/c%20d') to return false:

I find the same problem.When I use url-polyfill and pdfjs-dist in my web,the url-polyfill will lose efficacy. Because pdf-dist use this code to check if URL is valid.
         if (typeof URL === 'function' && _typeof(URL.prototype) === 'object' && 'origin' in URL.prototype) {            var u = new URL('b', 'http://a');         u.pathname = 'c%20d';         hasWorkingUrl = u.href === 'http://a/c%20d';       }
But in safari,
 u.href==='http://a/c%2520d'
And then the pdfjs-dist will rewrite URL.But the module doesn't achieve the searchParams method.fortunately the pdfjs-dist have  Stop exposing the URL polyfill in the global scope.
More over ,If this is a bug ? url-polyfill use this code to check URL is supported.But after execute the module .The check is false also in safari.
var checkIfURLIsSupported = function() {     try {       var u = new URL('b', 'http://a');       u.pathname = 'c%20d';       return (u.href === 'http://a/c%20d') && u.searchParams;     } catch(e) {       return false;     }   };

I don't have Safari on the hand. Could you fix it on your side, run the tests and create a pull request please ?

Currently looks like everything works fine (Safari 12.1.1):


FPE is sensitive to leg preference

Example: Subject 5, Trial 11. Prefers their left leg significantly and it's breaking the VPP model.

A lot of subjects do this actually? It's not a fault in the measurements I think, because the forceplace measured positions lie between the LML and MML optical markers at all times

Did fix with assymetrical model


Resource types / patterns for Path Item Objects

OData services mainly consist of "entity sets" which are collections of "entities" that are uniquely identified by their key property values within their entity set.
So a "read-only entity set" will support

GET on the set, with a predefined set of query options for paging ($top, $skip), filtering, sorting, free-text search, ...
GET on the individual entity by specifying the entity set plus the key property values

And "updatable entity sets" will in addition support

POST on the set to add new entities
PATCH and DELETE on individual entities

Basically an OData-aware client only needs the entity set name, the structure definition for each entity, and the "readonly/updatable" information to know the path templates and expected request and response bodies, allowed headers, possible response codes etc.
Spelling this out for OData-unaware clients takes a lot of highly repetitive content in the /paths section.
It would be great to have resource types or parameterized templates/patterns for Path Item objects that would allow to specify
/Products:
ReadOnlyEntitySetOfType
{ $ref: #/definitions/Product }
/People:
UpdatableEntitySetOfType
{ $ref: #/definitions/Person }
Please note that we foresee (well, actually have :-) services with dozens or hundreds of entity sets, so this would significantly reduce the size of the OpenAPI documents.

Parent issue: #560

@OAI/tdc removing from OpenAPI.Next proposal and making a future consideration.

Agreed.

See https://github.com/OAI/moonwalk/discussions/30

As noted in the previous comment, the best place to continue this is in the Moonwalk repository, in the linked discussion.  Closing out of this repo - @ralfhandl if you think there's something that we might consider here for 3.x please comment and I'll be happy to re-open.

Let's tackle it in Moonwalk first and then check whether we want to downport it to 3.x


Add pep8-naming and more pre-commit hooks

Relevant Issues

python-discord/organisation#138
python-discord/organisation#153
Closes https://github.com/python-discord/django-simple-bulma/issues/40
Description

New hooks were added for pre-commit and they will run in CI too. pep8-naming was added as a flake8 plugin to ensure names comply with PEP 8.
Hooks added
A couple of these hooks automatically apply fixes. However, they still report failure and leave any changes they make uncommitted. Therefore, the user has to commit the automatic fixes.

check-merge-conflict - Check for files that contain merge conflict strings.
check-yaml - Attempts to load all yaml files to verify syntax.
end-of-file-fixer - Makes sure files end in a newline and only a newline.
mixed-line-ending - Replaces mixed line endings with LF.
trailing-whitespace - Trims trailing whitespace.
python-check-blanket-noqa - Enforce that noqa annotations always occur with specific codes

Reasoning

Dev dependencies were updated and re-pinned because why not? They were getting old enough. I'm just following along with the other repos.
The pre-commit environment is cached to speed up builds.
To reduce redundancy and overhead in maintain dependencies, flake8 is now invoked through the current active Python environment rather than being installed in pre-commit's isolated environment. This means the hook now assumes the user already has flake8 installed i.e. via pip install -e .[dev].

Additional Details

The pre-commit venv is cached. There should only be a cache miss if the .pre-commit-config.yaml file changes or if the location of the Python interpreter changes (more realistically, if the Python version changes). Maybe Azure wipes the cache sometimes too, but in that case pre-commit will simply re-create the environment.

It would be nice to get this solved before we release v2.0.0.
I'm a little bit on the fence about it. On one hand, this has been convenient in other projects where it has been added. On the other hand, it's a little less applicable to this project (for example, we don't have any yaml files). More importantly, this PR was written before we switched to GitHub Actions, and has to be completely rewritten.
I guess I'd be open to a rewrite, but I don't really want to spend time on doing one myself. I think I could live without pre-commit in this project. We don't have a lot of contributors, and the few we have are quite experienced developers.
@MarkKoz, how do you feel about this?

I don't mind re-doing it in GH Actions @lemonsaurus. Also, we do have YAML files in the .github folder.

@MarkKoz that's true. Okay, if you'd like to re-do it for GHA I will gladly review it right away.

@lemonsaurus it's ready for a review now

@MarkKoz Now that we have merged the submodule refactor #56, this PR has a ton of conflicts. Could you clean up the branch so that it doesn't try to change any of the js, css, or sass files (which are no longer in this repo), and just have this PR contain the CI changes?


Use relative imports in SOCKS module.

Resolves #772.
It may be worth pushing out a 0.14.1 with this change in it at some point soon, but that's totally up to you @shazow.

0.14.1 -> 1.14.1 ;)

I am so tempted to just merge this. :tongue:

This doesn't affect urllib3 at all, right? Just for requests? Is it necessary to publish a release if it won't affect unvendored distributions?

No immediate rush. =)


Prevent k8s pods from accessing instance metadata

We're currently granting aws permissions to kubernetes nodes via iam roles, which means nodes look up their temporary access credentials via the aws metadata service. This is convenient and means we never have to generate or distribute credentials, but it also means that kubernetes containers can access the credentials via the host network (to verify, exec into a container and curl the aws metadata endpoint). Some non-user-facing containers that handle logging should have access to aws, but tenant containers should not.
Ideas:

Use kube2iam to assign necessary role to each pod
Use iptables to block pod access to aws metadata and pass credentials to pods as needed
Revert to iam users instead of roles and pass credentials to nodes and pods as needed

Discussion:
https://github.com/kubernetes/kubernetes/issues/23580
cc @cnelson @sharms

Based on discussion with @cnelson and @sharms, we're going to give kube2iam a try.


Fastlane hangs for 2-3 minutes before doing anything

At first I thought there no output until it was done, but turns out fastlane is just hanging for 2-3 minutes whenever I run anything and then the output starts happening. See the output below to see what I mean:
Alexs-Hack-Pro:Euchre Alex$ date && fastlane -v && date
Mon Nov 16 13:50:58 EST 2015
fastlane 1.39.0
Mon Nov 16 13:53:15 EST 2015

Have you tried running sudo gem cleanup ?

@xfreebird I have not yet. I'll give that a try later today and see if it helps things. This is a fairly fresh install though I believe. Only a few months old.

@alexargo I have four build machines running fastlane since september, and some of them are mac mini from 2009. I get 17s on a 2009 mac mini.
date && fastlane -v && date
Tue Nov 17 20:09:07 EET 2015
fastlane 1.39.0
Tue Nov 17 20:09:24 EET 2015

@xfreebird You are awesome! I'm not a rubyist so I would never have thought to do it. Is it mentioned somewhere in the docs to run it? Seems like the frequently release pace of fastlane et all necessitates running it. Here's some results from my Late 2014 Mac Mini
Before gem cleanup (7 min 27 sec):
date && fastlane -v && date
Tue Nov 17 13:13:49 EST 2015
fastlane 1.39.0
Tue Nov 17 13:21:22 EST 2015

After gem cleanup(2 sec):
Tue Nov 17 13:25:12 EST 2015
fastlane 1.39.0
Tue Nov 17 13:25:14 EST 2015�

@alexargo Actually the update message should tell you to run sudo gem cleanup every 5th time there is an update.
Also it is in the docs: https://github.com/fastlane/fastlane#installation

@KrauseFx Gotcha. Yeah I'm not sure what else there is you can do reasonably to inform us. I normally just update all my gems so the update message normally just gets lost. Is the time before you actually run fastlane something that can be measured? (That way you could notify when running fastlane if it's taking longer than expected)

I don't think that's possible.


Add executable bits to the file with shebang.

About examples/echo_server.rb
There is a shebang "#!/usr/bin/env ruby" in the file.
For this case, I think that executable bits (chmod +x) should be added to the file.
For example: See ruby repository.
https://github.com/ruby/ruby/tree/trunk/sample
The file with shebang, is executable, otherwise not.

Thanks!


Build failure on local Ubuntu (14.04), "Failed to find setuptools while building pex"

PEX may be Anaconda Python related using version 2.7.6, also re-installed Python using Docker Ubuntu 14.04 instructions including manual installation: python-setuptools.
INFO: From PexPython heron/shell/src/python/heron-shell.pex:
Traceback (most recent call last):
File "/home/tony/.cache/bazel/_bazel_tony/91ccb56ec8436235118164d4e388a63e/heron/bazel-out/local_linux-fastbuild/bin/3rdparty/pex/_pex.runfiles/3rdparty/pex/_pex.py", line 169, in
sys.exit(main())
File "/home/tony/.cache/bazel/_bazel_tony/91ccb56ec8436235118164d4e388a63e/heron/bazel-out/local_linux-fastbuild/bin/3rdparty/pex/_pex.runfiles/3rdparty/pex/_pex.py", line 162, in main
pex_builder.build(output)
File "/home/tony/.cache/bazel/_bazel_tony/91ccb56ec8436235118164d4e388a63e/heron/bazel-out/local_linux-fastbuild/bin/3rdparty/pex/_pex.runfiles/3rdparty/pex/pex/pex_builder.py", line 414, in build
self.freeze(bytecode_compile=bytecode_compile)
File "/home/tony/.cache/bazel/_bazel_tony/91ccb56ec8436235118164d4e388a63e/heron/bazel-out/local_linux-fastbuild/bin/3rdparty/pex/_pex.runfiles/3rdparty/pex/pex/pex_builder.py", line 398, in freeze
self._prepare_bootstrap()
File "/home/tony/.cache/bazel/_bazel_tony/91ccb56ec8436235118164d4e388a63e/heron/bazel-out/local_linux-fastbuild/bin/3rdparty/pex/_pex.runfiles/3rdparty/pex/pex/pex_builder.py", line 358, in _prepare_bootstrap
raise RuntimeError('Failed to find setuptools while building pex!')
RuntimeError: Failed to find setuptools while building pex!
ERROR: /home/tony/workspace/heron/heron/shell/src/python/BUILD:17:1: null failed: _pex failed: error executing command bazel-out/local_linux-fastbuild/bin/3rdparty/pex/_pex --entry-point heron.shell.src.python.main bazel-out/local_linux-fastbuild/bin/heron/shell/src/python/heron-shell.pex ... (remaining 1 argument(s) skipped).

is this still a problem?


Fix Mixing hparams and arguments in LightningModule

Before submitting

[x] Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
[x] Did you read the contributor guideline, Pull Request section?
[x] Did you make sure to update the docs?
[x] Did you write any new necessary tests?
[x] If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?
Fixes #1468
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃

As in allow things in kwargs to override model_args?

i say keep it simple as is now

@HenryJia can you update the docs to show this?

@williamFalcon Yep, done

@Borda Added note to changelog, looking into clean ways of adding a test for this...

The note you added in the documentation could be confusing. I wouldn't call these extra arguments hyperparameters. Because to me it reads like you can do load_from_checkpoint(file, learning_rate=2) and then magically somewhere in Lightningmodule access it with hparams like self.hparams.learning_rate. But clearly this is not what you try to achieve. You simply want to pass in some extra args, in addition to hparams dict. Apart from that, I don't see why this implementation prevents anyone passing in arguments that ARE part of the hparams dict.
A reformulation and an example in the docs would make it clear (at least for me).

@awaelchli Sounds good I'll change it slightly then. Regarding the example, htere already is one in the docs. It's the last example of that section.

Awesome, it's cristal clear now. Thanks for the fix.


Add current_timestamp() Presto function and an alias now()

Implement current_timestamp() to resolve issue https://github.com/facebookincubator/velox/issues/4494
.

@yiweiHeOSS this PR has the same issue as in https://github.com/facebookincubator/velox/pull/4653
You need to resolve that first.

@yiweiHeOSS this PR has the same issue as in #4653 You need to resolve that first.

Hi @majetideepak , do you think I have to use current_timestamp? Currently, my code is working well if I use get_current_timestamp.

Hi @yiweiHeOSS, Thanks for this function. Please can you rebase and resolve conflicts for the next round of reviews.

Hi @yiweiHeOSS, Thanks for this function. Please can you rebase and resolve conflicts for the next round of reviews.

Hi @aditi-pandit , I resolved the conflicts. Thanks.

I met one problem when run the pipeline "linux-adapters", it looks like some problem with CurrentDateFunction
https://app.circleci.com/pipelines/github/facebookincubator/velox/24720/workflows/7630c891-f297-40ad-a058-1df9cf287bc2/jobs/156765

The pipeline ran clean @aditi-pandit I believe it is good to review now. Thanks.

Hi @aditi-pandit , I fixed your comments again, what's the next step for this PR. I hope we can get it merge soon.

I fixed your comments again. Thank you @aditi-pandit .

@yiweiHeOSS this PR has the same issue as in #4653 You need to resolve that first.

Hi @majetideepak , do you think I have to use current_timestamp? Currently, my code is working well if I use get_current_timestamp.

@yiweiHeOSS yes, the function name must match the Presto function name. So get_current_timestamp will not match the invocation from Presto. You should verify by running Prestissimo.

@yiweiHeOSS this PR has the same issue as in #4653 You need to resolve that first.

Hi @majetideepak , do you think I have to use current_timestamp? Currently, my code is working well if I use get_current_timestamp.

@yiweiHeOSS yes, the function name must match the Presto function name. So get_current_timestamp will not match the invocation from Presto. You should verify by running Prestissimo.

Yes, please use the same function name,

@yiweiHeOSS this PR has the same issue as in #4653 You need to resolve that first.

Hi @majetideepak , do you think I have to use current_timestamp? Currently, my code is working well if I use get_current_timestamp.

@yiweiHeOSS yes, the function name must match the Presto function name. So get_current_timestamp will not match the invocation from Presto. You should verify by running Prestissimo.

Yes, please use the same function name,

  registerFunction<CurrentTimeStampFunction, TimestampWithTimezone>(
      {prefix + "current_timestamp", prefix + "now"});

@majetideepak If I change it to 'current_timestamp', I will get below error running the testcase:
E0605 21:26:40.212424  6928 Exceptions.h:68] Line: /root/velox/velox/parse/TypeResolver.cpp:108, Function:resolveScalarFunctionType, Expression:  Scalar function doesn't exist: get_current_timestamp., Source: USER, ErrorCode: INVALID_ARGUMENT

unknown file: Failure
C++ exception with description "Exception: VeloxUserError
Error Source: USER
Error Code: INVALID_ARGUMENT
Reason: Scalar function doesn't exist: get_current_timestamp.
Retriable: False
Function: resolveScalarFunctionType
File: /root/velox/velox/parse/TypeResolver.cpp
Line: 108
Stack trace:
Stack trace has been disabled.Use --velox_exception_user_stacktra
```ce

@yiweiHeOSS this is the same issue as discussed here https://github.com/facebookincubator/velox/pull/4653#issuecomment-1522195224

@yiweiHeOSS this is the same issue as discussed here #4653 (comment)

Hi @majetideepak , I asked @minhancao and maybe we need some guidance about how to do this part. So far our code works well with get_current_timestamp and 'get_current_time'. Could you please tell me how to do it? Thanks.

This is an issue due to the DuckDB parser we use to parse the expression strings in tests. It is consuming current_time and returning get_current_time.
The alternative is to manually build the expression tree. You will have to build core::TypedExprPtr directly by std::make_sharedcore::CallTypedExpr(returnType, args, name);
Let me know if you need help with this.

@yiweiHeOSS, below are some more pointers.
The test uses evaluate() whose arguments are the function name current_time along with the input data.
If you look at the evaluate() implementation, it first creates a core::TypedExprPtr by invoking makeTypedExpr with the function name and type as input.
https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/tests/utils/FunctionBaseTest.h#L84
The makeTypeExpr function first parses the string using DuckDB parser and then converts it into the Velox expression.
https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/tests/utils/FunctionBaseTest.h#L66
You have to skip this DuckDB parsing since that is the root cause of this issue.
You must instead manually build the Velox expression using the right function name i.e. current_timestamp.
Let me know if you have other questions.

@majetideepak Thank you for your instruction. Ok, I figured it out. I Change the registration name from 'get_current_timestamp' to 'curren_timestamp' and modify the testcase for it. It should be good to review. cc: @aditi-pandit

This PR is ready to review again @aditi-pandit @majetideepak. Thanks

Hi, @aditi-pandit. Can we move forward on this PR. I hope this PR can be delivered so I don't need to worry about it anymore.

Hi, @aditi-pandit. Can we move forward on this PR. I hope this PR can be delivered so I don't need to worry about it anymore.

@yiweiHeOSS : I have some questions about the test code. Let's live debug this together to write the test logic. Have messaged you.

The build failure is not from my changes. I also don't know how to rerun it except push a new commit. @aditi-pandit

@yiweiHeOSS : Please can you rebase. Your changes look good otherwise.

Thank you so much @aditi-pandit for helping me solve the problem of BaseVector/RowVector in testcase. This PR is ready to review again.
cc: @majetideepak

Ok, thanks @aditi-pandit . I applied this change and simplified the testcase and improved the comment. Please review it again.

Ok, I fixed that comments again. @aditi-pandit

As per https://github.com/facebookincubator/velox/pull/4656#discussion_r1344901362 we have decided to not add this function in Velox.


⚠ Bofrost.lu has degraded performance

In a810da8, Bofrost.lu (https://www.bofrost.lu) experienced degraded performance:

HTTP code: 200
Response time: 1138 ms

Resolved: Bofrost.lu performance has improved in 6cb0669 after 6 minutes.


Tkt164: leave email blank for tutorial accounts

Stop generating email addresses at gpolab.bbn.com for tutorial accounts. No one really gets those emails. Instead, leave the email address blank. As a result, when the tutorial attendee tries to log in to the portal, they will be required to self-assert their email address.
As a result, we'll know the real email address of the person attending the tutorial, and they can actually get the emails that the portal sends.
This change requires a schema change, allowing the email address to be null - be sure to apply update-4.sql. Also removed unused bin/geni-tutorial-accounts.py. This PR also modifies the email that the tutorial organizer gets, explaining that attendees must self assert their email address.
Fixes issue #164

updated this PR

collect the name of the organizer and include that in the account logs and the email to the organizer and admins
shorten the email to the organizer (don't describe editing profile)
basic error checking of organizer name and email


Remove Explore artwork categories on fair overview

Unfortunately this information is moving into a sub view (martsy) under the Works section meaning we really don't need to repeat it. Also find that many fairs only have one or two featured categories and the current UI doesn't accommodated this very well :/

Leaving in the pile for someone else to pick up, I'm back to doing Folio work

is there an easy way of telling when things make it into the current build or not? (I can tell this one didn't make it into what we shipped to apple but I was curious of how to confirm that)


Enhance memory allocation to improve transform performance

Is your feature request related to a problem? Please describe.
Currently, each row in the dataprep is being initialized with no initial size.
This increases the amount of unnecessary realloc and increases gc.
현재 dataprep의 각 row들이 초기 사이즈 없이 초기화되고 있습니다.
이렇게 하면 불필요한 realloc이 많아지고, gc가 증가하게 됩니다.
Describe the solution you'd like
All will be improved to have proper initial values.
모두 적절한 초기값을 가지도록 개선할 예정입니다.

This issue has little impact on performance.
We developed code to use Apache Spark (https://github.com/metatron-app/metatron-discovery/issues/2352).
This issue is not needed anymore.


Support defer loading of dataTables data

Adds support to defer the loading of dataTables data with AJAX. Applied to the sources table on the source query page, this stops the results table from being populated with all sources. The results table is only populated when the user submits a query using the form. If the user wants to view all sources, they can still do this by submitting an empty query.
Closes #629

Just thought, not sure if there is any wording in the docs that needs updating. I.e. it implies the results are already there.


simulation dataset support

Desired "dataset support", from our previous HedgeDoc.
Simulations:

[x] TNG (/virgotng/universe/IllustrisTNG/)
[x] EAGLE ("TNG-like" version on /virgotng/universe/Eagle/)
[x] EAGLE (original version)
[x] SIMBA ("TNG-like" version on /virgotng/universe/Simba/)
[x] SIMBA (original version)
[x] Illustris (/virgotng/universe/Illustris/)
[x] TNG-Cluster (/virgotng/mpia/TNG-Cluster/L680n8192TNG/)
[ ] MTNG (/virgotng/mpa/MTNG/)
[x] FIRE-2 (public simulations, on /virgotng/mpia/FIRE-2/)
[ ] FLAMINGO (need to get pre-publication access, and/or infer from current SWIFT code)
[x] Auriga (/vera/ptmp/gc/dnelson/public/auriga_halo6_lvl6/ for example)
[ ] THESAN (currently private, expected release in 2023, can ask Enrico Garaldi)

I suggest that MTNG and FLAMINGO support, at release, be partial, i.e. minimal.
Both contain exotic types of additional files/lightcones/catalogs/trees which will take time to properly consider, and this isn't necessary for day 1.

Added some more unit support in #96 . Generally all these simulations should load in their basic form (snapshot plus optional catalog).


⚠ API - Lens has degraded performance

In c5dfae3, API - Lens ($LENS_API_URL) experienced degraded performance:

HTTP code: 503
Response time: 372 ms

Resolved: API - Lens performance has improved in 9852161 after 6 minutes.


fail to launch : log.txt permission denied

Description of the problem or steps to reproduce
unable to launch : permission denied on log.txt
Specifications
micro-2.0.0-rc3
You can use micro -version to get the commit hash.
Commit hash:
OS: Slackware64-current
Terminal:xfce4-terminal
Compiled from source
It installs the binary to the GOPATH
but for the user, if the directory is not writable, micro can't produce the log.txt and will fail.
If the file cannot be avoided, let's create it in /tmp or in the user's home.

I have this issue in Fedora, it was build with:
%build
export LDFLAGS="-X main.Version=2.0.1 \
                -X main.CommitHash=7c71995 \
                -X 'main.CompileDate=February 17, 2020' "
for cmd in cmd/* ; do

  # https://bugzilla.redhat.com/show_bug.cgi?id=995136#c12
  GOPATH="/home/bob/rpmbuild/BUILD/micro-2.0.1/_build:${GOPATH:+${GOPATH}:}/usr/share/gocode" GO111MODULE=off \
  go build -buildmode pie -compiler gc -tags="rpm_crashtraceback ${BUILDTAGS:-}" -ldflags "${LDFLAGS:-}-X github.com/zyedidia/micro/version=2.0.1 -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '" -a -v -x -o /home/bob/rpmbuild/BUILD/micro-2.0.1/_build/bin/$(basename $cmd) github.com/zyedidia/micro/$cmd;

done

Ah I see the issue since it wasn’t built with the makefile. I’ll change the default when using go build to not enable debug mode when I get the chance. In the mean time you can run with micro -debug=false.

No worry I changed my LDFLAGS to match the Makefile
%build
export LDFLAGS="-X 'github.com/zyedidia/micro/internal/util.Version=%{version}' \
                -X 'github.com/zyedidia/micro/internal/util.CommitHash=%{shortcommit}' \
                -X 'github.com/zyedidia/micro/internal/util.CompileDate=%{compiledate}' \
                -X 'github.com/zyedidia/micro/internal/util.Debug=OFF'"

for cmd in cmd/* ; do
  %gobuild -o %{gobuilddir}/bin/$(basename $cmd) %{goipath}/$cmd
done

On Ubuntu a debug log is also created with the latest update (2.0.1-1)...


[sdl2] Update to 2.0.14

Library name: SDL2
New version number: 2.0.14
Other information that may be useful (release notes, etc...)
Release notes are on the Mercurial repo.
I'd work on this PR myself, but I'm busy at the moment, and I don't know what each patch is supposed to do and whether they will be necessary anymore. If anyone wants to update it sooner, then go ahead.

Please also fix minor issue with copyright line in resource:

I didn't had such issue when building manually via CMake.

I investigated this encoding issue a bit.
It is because vcpkg adding -c65001 into CMAKE_RC_FLAGS but src\main\windows\version.rc file is in CP1252. Just need patch it and convert to UTF-8.

Solved by #15398

Solved by #15398