🐛 Bug:  All Contributors comments are valid and yet also trigger "I couldn't determine any contributions to add"

Bug Report Checklist

[X] I have tried restarting my IDE and the issue persists.
[X] I have pulled the latest main branch of the repository.
[X] I have searched for related issues and found none that matched my issue.

Expected
This repo should use https://github.com/JoshuaKGoldberg/all-contributors-auto-action to post All Contributors comments that work and are valid.
Actual
Comments create responses like https://github.com/JoshuaKGoldberg/create-typescript-app/pull/429#issuecomment-1546457872 from the @allcontributors bot:

@JoshuaKGoldberg
I've put up a pull request to add @JoshuaKGoldberg! 🎉
I couldn't determine any contributions to add, did you specify any contributions? Please make sure to use valid contribution names.

Additional Info
Filing an issue on this repo for tracking. The root issue is tracked in https://github.com/all-contributors/app/issues/468.

This was fixed. 🚀


SVG Dimensions are incorrect

The SVG files are set to 512mm (1935.1182px) and not pixels.
This makes Godot import them at the full 1935x1935 pixels instead of 512x512 making the cached texture be 10MiB instead of 760KiB (a drastic difference).

Fixed, thanks!

Resolved 4a0a2c76743e77606e2c31da904eeababef7450d


Getting error when running build runner command

Error
INFO] Generating build script...
[INFO] Generating build script completed, took 474ms

[INFO] Precompiling build script......
[WARNING] ../../Development/flutter/.pub-cache/hosted/pub.dartlang.org/isar-2.2.1/lib/src/isar_connect.dart:97:13: Error: The argument type 'Future<void>?' can't be assigned to the parameter type 'Future<void>' because 'Future<void>?' is nullable and 'Future<void>' isn't.
 - 'Future' is from 'dart:async'.
  unawaited(_watchSubscription?.cancel());
            ^
[INFO] Precompiling build script... completed, took 888ms

[SEVERE] Failed to precompile build script .dart_tool/build/entrypoint/build.dart.
This is likely caused by a misconfigured builder definition.

pub finished with exit code 78

Code sample
import 'package:isar/isar.dart';
part 'picked_file_model.g.dart';

@Collection()
class PickedFileModel {
  int id = Isar.autoIncrement;
  final String path;
  final String name;
  final String data;

  PickedFileModel(this.path, this.name, this.data);
}

Version

Flutter version: 2.5.3
Isar version: 2.2.1
Isar Generator version: 2.2.1

issue resolved
its now working after updating flutter to latest version i.e  2.10.2


Johannes/documentation

Added documentation of the function calls to the APIWrapper

This is a great addition thanks. Is there a reason you didn't use doc comments on the methods?

I hadn't decided yet if internal documentation should be doc comments or just comments. I figured once the info was there, that'd be simple to refacture.


Package 4.0.0 that is mentioned in README is not released.

The package version 4.0.0 that is mentioned in the README is not available as a nuget package on nuget.org.
The last version on nuget.org is 4.0.0-rc.4

Hi @mrt181,
I've just released it.


Task id is always 0

Modifying the trace example like this:
use async_log::{span};
use log::info;

fn setup_logger() {
    let logger = femme::pretty::Logger::new();
    async_log::Logger::wrap(logger, || /* get the task id here */ 0)
        .start(log::LevelFilter::Trace)
        .unwrap();
}

fn main() {
    setup_logger();

    let task1 = async { span!( "level {}", 1,
    {
        info!( "From task 1" );

    })};

    let task2 = async { span!( "level {}", 1,
    {
        info!( "From task 2" );

    })};

    let task3 = async { span!( "level {}", 1,
    {
        info!( "From task 3" );

    })};

    async_std::task::spawn( task1 );
    async_std::task::spawn( task2 );
    async_std::task::spawn( task3 );

    std::thread::sleep( std::time::Duration::from_millis( 500 ) );
}

I get the following output (as you can see, the task_id is always 0 for the relevant log messages, and log messages from async-std have task_id twice):
car --example trace
   Compiling async-log v2.0.0 (/data/doc/code/RUST/async-log)
    Finished dev [unoptimized + debuginfo] target(s) in 0.80s
     Running `/tmp/rs/debug/examples/trace`
●  spawn
   /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.5.0/src/task/builder.rs:41
   › task_id: 1
   › parent_task_id: 0
   › thread_id: 1
   › task_id: 0

●  spawn
   /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.5.0/src/task/builder.rs:41
   › task_id: 2
   › parent_task_id: 0
   › thread_id: 1
   › task_id: 0

◯  level 1
   src/lib.rs:188
   › span_mark: "start"
   › thread_id: 4
   › task_id: 0

●  spawn
   /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.5.0/src/task/builder.rs:41
   › task_id: 3
   › parent_task_id: 0
   › thread_id: 1
   › task_id: 0

◯  level 1
   src/lib.rs:188
   › span_mark: "start"
   › thread_id: 18
   › task_id: 0

●  From task 1
   examples/trace.rs:16
   › thread_id: 4
   › task_id: 0

◯  level 1
   src/lib.rs:188
   › span_mark: "start"
   › thread_id: 21
   › task_id: 0

●  From task 2
   examples/trace.rs:22
   › thread_id: 18
   › task_id: 0

◯  level 1
   src/lib.rs:188
   › span_mark: "end"
   › thread_id: 4
   › task_id: 0

●  From task 3
   examples/trace.rs:28
   › thread_id: 21
   › task_id: 0

◯  level 1
   src/lib.rs:188
   › span_mark: "end"
   › thread_id: 18
   › task_id: 0

●  completed
   /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.5.0/src/task/builder.rs:57
   › task_id: 1
   › thread_id: 4
   › task_id: 0

◯  level 1
   src/lib.rs:188
   › span_mark: "end"
   › thread_id: 21
   › task_id: 0

●  completed
   /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.5.0/src/task/builder.rs:57
   › task_id: 2
   › thread_id: 18
   › task_id: 0

●  completed
   /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.5.0/src/task/builder.rs:57
   › task_id: 3
   › thread_id: 21
   › task_id: 0

I missed the /* get the task id here */ part. Didn't realize that getting the task Id was not included in the crate.

@najamelan I started a patch to make it work out of the box in https://github.com/async-rs/async-log/pull/9, but never saw it through to completion. Still unsure if it's a good idea.

Looks like that is async-std specific? I found a solution that works for me with tracing-futures. The Instrumented type wraps futures and executors to set the right span in the poll function. Thus it works even when using join. Everything that runs concurrently can be detected in the logs, making it possible to create a gui that puts the log statements in columns side by side. Haven't made such a gui yet, but I will soon.

Oh, and as an added advantage, that allows you to set any structured data, like a name instead of a numeric task id, which is to be preferred I think.


version should be between apostrophes  ''

when you directly copy the resulting compose file and try to run error Version in "./docker-compose.yml" is invalid - it should be a string. is raised due lack of apostrophes in the version section

Thanks for the report @kassanmoor! Would you like to try taking a stab at a fix for this?

I want @magicmark but I guees I need some instructions, I have the repo, create my branch and do npm install but get npm WARN<EMAIL_ADDRESS>No repository field, did not touch node before, do you have a hackme/build me or readme instructions somewhere?

Closing as fixed with the latest version (1.0.4) :)


Rails/SaveBang "check used in conditional" false positive

The Rails/SaveBang cop in v0.74.0 emits a false positive for the "check used in conditional" rule, where the save-like call is actually in the if-body, not the condition.

Expected behavior
I expect Rails/SaveBang to suggest I use create! instead of create, like so:
test.rb:1:12: C: Rails/SaveBang: Use create! instead of create if the return value is not checked.
some_model.create if condition
           ^^^^^^

Actual behavior
I get a message about the save-like call being used as a conditional:
test.rb:1:12: C: Rails/SaveBang: create returns a model which is always truthy.
some_model.create if condition
           ^^^^^^

Steps to reproduce the problem

Create a file (I named it test.rb) with the following minimum reproducible case code:

some_model.create if condition

Execute rubocop test.rb
Observe incorrect error message

RuboCop version
$ bundle exec rubocop -V
0.74.0 (using Parser <IP_ADDRESS>, running on ruby 2.6.3 x86_64-darwin18)

FYI this applies to save-like calls in any branch body of a conditional. Examples:
if condition
  ...
elsif
  model.create
        ^^^^^^
else
  model.create
        ^^^^^^
end


Updating gradle to V3.3 makes Eclipse unable to resolve dependencies.

Eclipse v4.11.0 doesn't import the gradle dependencies so the java classes show the error 'the import *** cannot be resolved'.
Expected Behavior
Gradle should import all the libraries and be shown in the 'Gradle Dependencies' classpath container
Current Behavior
Gradle task like 'refresh dependencies','refresh all' or 'build' are finishing ok but dependencies are not shown in eclipse and java classes are in error because unresolved imports.
Context

It was working with gradle v2.4. Upgrading to gradle v3.3 made it fail. I've read that in recent versions of eclipse the method 'compile' is deprecated, but I tried 'api' and 'implementation' but it doesn't work.
Steps to Reproduce

apply plugin: 'java'
apply plugin: 'eclipse'
apply plugin: 'eclipse-wtp'
apply plugin: 'jacoco'
dependencies{
	// Proyectos
	// Librerias
	compile project.ext["commons"]
	compile project.ext["cxf.foundation"]
	compile project.ext["ehcache"]
	compile project.ext["log4j"]
	compile project.ext["xml"]
	compile project.ext["javaee"]

}
These project.ext["..."] are filetrees defined in the root project.
Your Environment

Eclipse IDE for Enterprise Java Developers.
Version: 2019-03 (4.11.0)
Build id: 20190314-1200
Gradle version: 3.3
The project works with Gradle STS.
Gradle buildship plugin for eclipse version 3.0

Are you importing the project with Buildship directly or are you generating the Eclipse metadata files with gradle eclipse and then importing that?
Is it possible for you to try a newer version of Gradle? 5.4.1 is the latest.
You can try getting more help on our forums: https://discuss.gradle.org/

Hi, thanks for answering my questions. I'm importing the project directly
with buildship, I've tried with newest versions (including 5.4.1) but it
doesn't work. I think the problem might be that the metadata generated by
eclipse was generated before with gradle STS, and it's supported by
springsource (not buildship) but also I tried the task clean eclipse and
deleting this files and none worked. Plus, I tried changing the nature of
the project and even uninstalling the STS plugin but it doesn't work.
PD: Android Studio and IntelliJ resolve the dependencies without any
problem or additional stuffs.
Thank you!
On Thu, Jun 27, 2019, 5:44 PM Sterling Greene<EMAIL_ADDRESS>wrote:

Are you importing the project with Buildship directly or are you
generating the Eclipse metadata files with gradle eclipse and then
importing that?
Is it possible for you to try a newer version of Gradle? 5.4.1 is the
latest.
You can try getting more help on our forums: https://discuss.gradle.org/
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/gradle/gradle/issues/9799?email_source=notifications&email_token=AARE4QHWHUMCFUBHTEQGSJ3P4U7EHA5CNFSM4H3VYYWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYYSLEQ#issuecomment-506537362,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARE4QB5VWJYXNCGPE7RLELP4U7EHANCNFSM4H3VYYWA
.


Automated cherry pick of #9490: Hotfix/baremetal pd makegood

Cherry pick of #9490 on release/3.6.
#9490: Hotfix/baremetal pd makegood

/lgtm

/approve


Target Framework is incorrect

It appears that 0.6.1 was compiled with the .NET Target Framework set to 4.6.1 instead of 3.5 like it should. This throws an error in the KSP.log

Thanks for catching this issue. I didn't even realize this was something to look out for. It's fixed in the latest commit


Issue1750: Curvilinear Raycaster crashes on AMD (Mesa drivers) + Ubuntu

Fix #1750
I've added some comments to the issue

The code will work with any tree depth, it can only affect performance. My code skips the first few levels since they generally don't give any performance improvements and they provide the compiler with less complex code. In the case of the Mesa driver, it would attempt to compile the largest search trees even if they weren't being used while other compilers only compile the ones being used.


Fix 1116448: Create new Blazor WebAssembly app and open Counter.razor…

…, you get squiggles about ambiguity related to currentCount variable
Problem was that if file was opened before project was loaded ProcessOpen was not called.

We're not sure this fixes the issue, but something we saw where Windows relies on this in the scenario, so we decided to add this so we can debug further if this doesn't fix it.

The scenario is: have Counter.razor open without a solution, then load the solution that contains it. On Windows it works smoothly, but on Mac it doesn't.
We don't see ProcessOpen() getting called because this check fails:
https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.VisualStudio.Editor.Razor/Documents/EditorDocumentManagerBase.cs#L156
And so later ProcessClose() is getting called without the matching ProcessOpen, which results in this: https://github.com/dotnet/aspnetcore/issues/18788

We're also not sure if we're also going to need the tracking logic here:
https://github.com/dotnet/aspnetcore-tooling/blob/master/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Documents/VisualStudioEditorDocumentManager.cs#L92


When debugging sometimes getting, fatal error: [SwiftFlowRouter]:

When debugging and hitting breakpoints during active route changes, sometimes I get the following fatal error:
fatal error: [SwiftFlowRouter]: Router is stuck waiting for a completion handler to be called. Ensure that you have called the  completion handler in each Routable element.

@avdwerff Uh, yes. This is indeed annoying during debugging. I think we should replace it with a warning  that people can use to create conditional breakpoints.
I added the fatalError in order to make folks aware that they forgot to call the completionBlock when switching to a new route. But I'll look into a better way of accomplishing this.
Thanks for making me aware!

@Ben-G Maybe completionBlock should be optional at all like most of other completion handlers. 🤔


Polymer 3.0 doesn't play nice with firebase serve at all

Description
Using Firebase hosting for a polymer app is much more difficult with Polymer 3.0 than previous version:
simply because we can not use firebase serve anymore.
The app needs to be served by polymer serve to rewrite every package import (which is a horrible useless step imho).
The problem lays with running a server with the correct settings such as Url rewrites or redirections, file serving settings, etc...
We have 2 solutions for running the app, however, neither of them is taking in account all of these requirement, and more than that, they don't allow middleware that could solve this issue, or interact with each other to solve this:
polymer serve will break the server settings and only correctly edit the code live...
firebase serve will correctly serves the files and urls, but not rewrite the code, so it will not work.
That's pretty awesome to make new stuff, but don't go saying that we can use Firebase if we can't even ran it for tests. If I 'build' my app statically every time I want to test something it would takes useless hours for nothing, simply not thinkable.
Live Demo
Every polymer 3 app hosted on Firebase hosting, using firebase hosting features (such as Url rewrites, redirections, etc...)
Steps to Reproduce

Write a Polymer 3 app
Use Firebase hosting with Urls rewrites
Try running your app locally (testing)

either using polymer serve
or firebase serve

Expected Results
App run fine, following hosting settings such as Url rewrites and every other settings allowed by Firebase hosting.
Actual Results
When running polymer serve your app will actually boot-up, but all the logic is broken and you can not test it as expected.
When running firebase serve your app files are correctly served according to hosting logic, but the code will break on import, as Javascript can not import node package syntax.
Browsers Affected

[x ] Chrome
[ x] Firefox
[ x] Edge
[ x] Safari 11
[ x] Safari 10
[ x] IE 11

Versions

Polymer: v3.x

This was a hard decision to make, but given our move to the NPM ecosystem and their convention of using named imports, we had to accommodate. We have a more in-depth discussion at https://github.com/Polymer/polymer/issues/5238#issuecomment-390012108 which also lists the tool empathy that we are experimenting with. This will run for all of your NPM dependencies on install time, so that you can use paths during development. Would that work out for you?

One alternative is to polymer build and then firebase serve those assets. I was already doing that with my Polymer 2 project, so it’s been a pretty simple transition. If you wrap the commands together in an NPM script, it’s even simpler. I go so far as to gather updated dependencies at the same time via my script to make sure I’m 100% up-to-date with each serve.

@TimvdLippe : There is indeed some inconsistencies between JS front-end and using NPM (I'm not questioning the choice of respecting NPM rule, rather the answer of --> use polymer serve / re-build your app every time.)
I think that the empathy tool, by one-time rewriting the import will fix this issue completely.
However it needs to feel solid enough for me to commit to a whole shift on Polymer 3 / empathy / NPM (though it feels natural described like this)
Speaking for myself here ;) but I feel that everyone having to make those choices for a production project will be in the same seat.
I will check this well named empathy tool to get a clearer view !
@Westbrook : That is totally not an option.
Reasons are simple:

It takes more than 30sec to build the app. It's a webapp, not that complex, so there are bound to be more complex project out there.
I'm already not using polymer build or serve or anything. Having the needs for complete control over the process, the build is using polymer-build

60 seconds to do a build seems incredibly long. Especially when developing there should be some caching to speed things up and shorten the edit / update cycle - 'how' will depend on the exact build tooling but most of the big name ones have some options that can help.
It's a shame that the module imports don't work like normal http requests, I don't understand the reason for not having the pathless imports supported - otherwise the resolution could be handled at the server by having a thin layer of node-resolution middleware, much less intrusive than having to parse and re-write content.
But I believe the re-writing part is middleware so the solution is to combine this with the firebase (or any other) hosting for development if you can't speed up the build (or don't want to). Maybe this could be using the proxy & live re-loading features of one system (e.g. browser-sync) working with firebase serve.
I don't think adding more Polymer specific tooling is the right solution.

I agree that it's a long time to build. Though as a small team we can't really spend all our time on refactoring the build process every time there is a new tooling set out-there (though we try to keep up).
So currently it's working, but takes time (even 30sec would still be too much [for live reloading] actually).

As @TimvdLippe said, the choice to adopt using npm package names for dependencies rather than paths was a difficult decision, but one we feel has more upside than downside, especially in light of a platform-based solution coming on the horizon.
That said, this is a decision we're not currently re-considering. In the interim, we're in the same boat as literally all other mainstream front-end development workflows, meaning some sort of transformation is required between npm dependency install and running in the browser.  However, we have the benefit of only requiring a very light transformation to make them loadable (names to paths) which we expect will go away in the future.
The empathy post-install tool seems like the ideal fit for your use case, since it mimics an experience similar to what package-name-maps will ultimately provide.
I'm closing this issue since there is nothing in the polymer core codebase we would do to address your issue.


[🐞] Error on running the application in deploy mode

Which component is affected?
Qwik Runtime
Describe the bug
I'm trying to move some components to separate component libraries outside of the main src directory. The goal is to create an application by micro frontend structure.
The problem is under building in SSR mode for production and running the application in deploy mode on  the express adapter. Everything is running when the application is running by vite but when it comes to running by node with npm run deploy command, there is an error:
Error: Optimizer should replace all usages of $() with some special syntax. If you need to create a QRL manually, use inlinedQrl() instead.     at async ESMLoader.import (https://qwikstarternby82h-em22.w-credentialless.staticblitz.com/blitz.d4a32cb71496f1cb7595ed7c8471aaa3b79d22fc.js:6:1209137)     at async i.loadESM (https://qwikstarternby82h-em22.w-credentialless.staticblitz.com/blitz.d4a32cb71496f1cb7595ed7c8471aaa3b79d22fc.js:6:246622)     at async handleMainPromise (https://qwikstarternby82h-em22.w-credentialless.staticblitz.com/blitz.d4a32cb71496f1cb7595ed7c8471aaa3b79d22fc.js:6:989144)
I can't figure out what is the problem. I checked all built assets and the component with qrl instance is created. But can't understand what is the solution for solving this.
Thanks for your collaboration.
Reproduction
https://stackblitz.com/edit/qwik-starter-nby82h
Steps to reproduce
The flow for creating steps are these:

npm create qwik@latest to create a new application
cd to application, make packages directory and cd into it
npm create qwik@latest to create a pure resumable component library
Add @packages/logo to tsconfig.json paths for importing path
Add <Logo /> to layout of main application
npm run build
npm run deploy

The application in the reproduction URL is built by the above steps and you can see the error by running these:

npm run build
npm run deploy

System Info
System:
    OS: Linux 5.13 elementary OS 6.1 Jólnir
    CPU: (4) x64 Intel(R) Core(TM) i7-4510U CPU @ 2.00GHz
    Memory: 6.63 GB / 15.52 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 16.13.2 - ~/.nvm/versions/node/v16.13.2/bin/node
    Yarn: 1.22.17 - ~/.nvm/versions/node/v16.13.2/bin/yarn
    npm: 8.1.2 - ~/.nvm/versions/node/v16.13.2/bin/npm
  Browsers:
    Brave Browser: <IP_ADDRESS>
    Chrome: 110.0.5481.100
  npmPackages:
    @builder.io/partytown: ^0.7.0 => 0.7.5
    @builder.io/qwik: 0.20.1 => 0.20.1
    @builder.io/qwik-city: 0.5.2 => 0.5.2
    undici: 5.20.0 => 5.20.0
    vite: 4.1.4 => 4.1.4

Additional Information
No response

@robisim74 I think there is a problem regarding resolve tsconfigPaths() because it's responsibility is handling paths. But it's wired that we should handle two separated sources for a simple issue.

I'm closing this since the core problem is about vite configuration. But would be appreciated if qwik consider this in any way.
Thanks.

@saeedhbi I'm sorry, the solution I provided only works if your package is in src, or if you import functions that don't use Qwik. Maybe you should reconsider keeping this issue open, because:

QwikVite has only one srcDir
vendorRoots seems not working
tsconfigPaths projects option is not enough
Vite config is not enough

@robisim74 I wanted to walk through the idea later. Thanks for informing me.
I will open the issue again since the idea will not resolve my problem.

@saeedhbi I found that vendorRoots wasn't working on my Windows machine for the usual path normalization problem... this should be the solution across systems (tested in Windows & Ubuntu):
     qwikVite({
      vendorRoots: [
        fileURLToPath(new URL('./packages/logo/src', import.meta.url))
      ]
    }),

https://stackblitz.com/edit/qwik-starter-kuzkre?file=vite.config.ts

Yes, It's working. Thanks. It saved me a lot of time.


[SPARK-44513][BUILD] Upgrade snappy-java to <IP_ADDRESS>

What changes were proposed in this pull request?
The pr aims to upgrade snappy-java from  <IP_ADDRESS> to <IP_ADDRESS>.
Why are the changes needed?
1.The newest version include a bug fixed:

Fix the GLIBC_2.32 not found issue of libsnappyjava.so in certain Linux distributions on s390x by @kun-lu20 in https://github.com/xerial/snappy-java/pull/481

2.Release notes:
https://github.com/xerial/snappy-java/releases/tag/v<IP_ADDRESS>
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass GA.

thanks @panbingkun @LuciferYang , merged to master and 3.5

Could we also backport to branch-3.4?


Don't adjust the HUBCollectionView insets.

Before
After

Codecov Report

Merging #351 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #351   +/-   ##
=====================================
  Coverage       0%     0%
=====================================
  Files           7      7
  Lines         132    132
=====================================
  Misses        132    132

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 54f336c...e20da20. Read the comment docs.

@krris - Is this a spotify-specific change, or something that belongs in the framework (I don't know too much about the problem). I'm just wondering if setting UIScrollViewContentInsetAdjustmentNever will prevent iOS11-specific layout behaviour on other users of the framework.

Right, it may impact other users of the framework.


Ported to Python 3

This PR ports this nice DDNS client for Cloudflare to Python 3.

Thanks for the contribution. Is it possible to make this patch backwards compatible with Python 2?

The only thing that was breaking compatibility with Python 2 was the difference between input and raw_input. This is fixed by binding the raw_input function to input for Python 2.


Rebase #3833

Rebases #3833 onto the latest master, allowing it to be merged.

@homu r+

:pushpin: Commit 1dd8adf has been approved by indirect

:hourglass: Testing commit 1dd8adf with merge 62f2e7a...

:broken_heart: Test failed - status

@homu r+

:pushpin: Commit 8444564 has been approved by indirect

:hourglass: Testing commit 8444564 with merge d40fde7...

:sunny: Test successful - status


Fix docs build warning from myst

Summary

Fixed cross-reference links on the docs.
Added build-docs testenv to tox.ini

../datumaro$ tox -e build-docs

How to test

Checklist

[ ] I have added unit tests to cover my changes.
[ ] I have added integration tests to cover my changes.
[ ] I have added the description of my changes into CHANGELOG.
[ ] I have updated the documentation accordingly

License

[ ] I submit my code changes under the same MIT License that covers the project.
Feel free to contact the maintainers if that's a concern.
[ ] I have updated the license header for each file (see an example below).

# Copyright (C) 2023 Intel Corporation
#
# SPDX-License-Identifier: MIT

Codecov Report
Patch coverage: 28.57% and project coverage change: -0.01 :warning:

Comparison is base (d122ab8) 78.88% compared to head (fcddaac) 78.87%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #922      +/-   ##
===========================================
- Coverage    78.88%   78.87%   -0.01%
===========================================
  Files          215      215
  Lines        25220    25220
  Branches      5046     5046
===========================================
- Hits         19895    19893       -2
- Misses        4191     4192       +1
- Partials      1134     1135       +1

Flag
Coverage Δ

macos-11_Python-3.8
?

ubuntu-20.04_Python-3.8
78.87% <28.57%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

datumaro/cli/util/__init__.py
26.22% <ø> (ø)

datumaro/cli/util/project.py
33.67% <ø> (ø)

...ugins/data_formats/common_semantic_segmentation.py
82.02% <ø> (ø)

...maro/plugins/accuracy_checker_plugin/details/ac.py
5.71% <16.66%> (ø)

.../accuracy_checker_plugin/details/representation.py
12.50% <100.00%> (ø)

... and 1 file with indirect coverage changes
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


Problems with types in the problem expert

The types in the problem expert does not seem to be correct. Indeed, although they are specified correctly in the terminal, the problem.pddl file generated is wrong and causes popf to go in seg fault!
The source code that show the problem: assignment_3_5.zip
ros2 launch assignment_3_5 plansys2_simple_example_launch.py  on terminal one
ros2 run plansys2_terminal plansys2_terminal
then in the terminal execute the commands
set instance capacity_4 capacity_number
source assignment_3_5/launch/commands
If you then issue a get plan the answer is no plan, but an investigation show that popf goes seg fault since the problem file is not precise.
The generated (correct) domain.pddl file domain.pddl.txt
The generated (wrong) problem.pddl file problem.pddl.txt
The plan file generated when popf is called plan.txt
The commands: commands.txt
The correct problem file to generate: good-problem.pddl.txt

Hi @roveri-marco
I see that the bug is that it uses the base type instead the derived type of an instance, right?

Not sure it is only that...

I tried to use the domain attached, but I can't run on the plansys2 terminal "set predicate ( at dr1 wp4)". Since I have had a similar issue with another simple domain I think the reason is that the problem expert can't understand the hierarchy among types. Could it be?
domain_logistics.pddl.txt
problem_logistics.pddl.txt

We can optimize the output to remove the function if no functions declared. It should be possible. I'll take care in one of the possible next pull request.

It should work... Can you share the output?

The output of the terminal is:  Failing to open file "assignment_3_5/launch/commands"


Sample call to create a new Subscription does not work without __metadata.type

Creating a new subscription by posting to the /subscriptions endpoint under a list appears to require the body of the request contain "__metadata": {"type":"Microsoft.SharePoint.Webhooks.Subscription"} in addition to values for resource, notificationUrl, and expirationDateTime.  Otherwise, SharePoint returns:
"{"error":{"code":"-1, Microsoft.SharePoint.Client.InvalidClientQueryException","message":{"lang":"en-US","value":"
An entry without a type name was found, but no expected type was specified.
To allow entries without type information, the expected type must also be specified when the model is specified."}}}"

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: dc58af9c-d9b8-5ed0-6117-2db7a1d425de
Version Independent ID: 7cc8696c-5946-0414-b5be-c7561d0e31b5
Content: Create a new subscription
Content Source: docs/apis/webhooks/lists/create-subscription.md
Product: sharepoint
GitHub Login: @VesaJuvonen
Microsoft Alias: vesaj

Hi, are you passing application/json;odata=verbose?
I think if you are passing a verbose header, you need to specify the __metadata attribute.

You are correct that I am passing ;odata=verbose.  I've got general purpose method for accessing SharePoint that deals with token authentication and takes a parameter for the HTTP method verb plus additional parameters as needed.  But it always passes Content-Type:application/json;odata=verbose in the headers.


SLC Form should clarify when the instructors' list is empty

When we create an SLC form, on the second page, where we need to select the instructor, there should be more clarification added. As of now, if the instructor for a course is not selected, it won't let the user continue to the third page. However, to select the instuctor, the user needs to click on the names appearing when they start typing the name of the instructor. If they don't click on the option with the instructor's name, and type the full name by themselves, it appears that no instructor is selected, and it is not clear for the user why they can't proceed to the next page. There should be a text in the empty table telling the user that they need to select the instructor and the table is empty

 SLC Form should clarify when the instructors' list is empty

 CCE Minor engagement options


ERROR

CANNOT START BOT GETTING AN ERROR

IN REPLIT

Can you be more precicely.. What error do you encounter?

Exactly!

Please re-open your issue based on a issue template!


GD-204: Minior update on contributing page

Why

There were some comments on #207

What

introduce CR commends from #207
improve used links to be opened in a new tab

I miss the direct linking to the steps but that is your call.
a link from where?

And you project(s) page https://github.com/users/MikeSchulze/projects/ ?
what you mean in detail?
The PR has no affect to the projects, only issues are linked to projects

I somehow did not get notification on this or just missed it.
Yours
a link from where?
I can link to https://github.com/MikeSchulze/gdUnit4/blob/master/CONTRIBUTING.md#github-flow but not on the list items. If you change

Select an open issue to work on or create a new issue if none exists.

into H3

Select an open issue to work on or create a new issue if none exists.

you can friendly point a user to that bullet
Your
what you mean in detail?
This PR is to teach your users ... maybe they want to discover your project too. But that can go into the README.md better?


Animated example breaks with multiple dialogs

When rendering multiple modal dialogs based on the example in https://github.com/cloudflare/react-modal2/blob/master/example/animated/script.js only one of them will ever work, as only a single component can be rendered into a given GatewayDest. This applies even if one one modal is ever opened at a time, so long as there is more than one instance of the app-specific Modal class.
As a workaround I'm using CSSTransitionGroup as the tagName for GatewayDest so that the transition group is always rendered, which works as long as at most one modal is rendered into it at any given time.
Here's the code I'm using
const Modal = propTypes({
  onBackgroundClick: PropTypes.func,
  show: PropTypes.bool.isRequired,
  children: PropTypes.node,
  className: PropTypes.string
})(defaultProps({
  onBackgroundClick: event => void 0
})(props => {
  const { show, children, className, onBackgroundClick } = props;
  return show ? <Gateway into='modal'>
    <ReactModal
      onClose={onBackgroundClick}
      modalClassName={classNames('reveal-modal', className)}
      backdropClassName='reveal-modal-bg'
    >
      { children }
    </ReactModal>
  </Gateway> : <div/>;
}));

export class ModalWrapper extends Component {

  static init() {
    ReactModal.getApplicationElement = () => document.getElementById('app-content');
  }

  render() {
    return <GatewayProvider>
      <application>
        <div id="app-content">
          {this.props.children}
        </div>
        <GatewayDest
          name="modal"
          className="modal-container"
          tagName={TransitionGroup}
          transitionName='modal'
          transitionEnterTimeout={300}
          transitionLeaveTimeout={300}
        />
      </application>
    </GatewayProvider>;
  }
}

Note that the above produces a bogus prop type warning in development mode because the tagName proptype is set as string even though any value suitable for React.createElement is accepted.

I'm not using react-modal2 (building a similar-ish thing), but I'm trying to solve this same problem. Any luck, @aij ?
One solution that worked for my Alerts system involved only having a single Alert instance for the entire app. I built it this way because new alerts are not necessarily immediately displayed: they're queued until the old one disappears or is dismissed. This is more complex than just rendering Alerts wherever, and it has some drawbacks, like only serializable data can be passed to the Alert. I don't really want to do the same system for modals just for the benefit of being able to animate them, but it is an option that's out there.
@thejameskyle, did you ever need to support animations for things rendered in gateways?

@jmeas We ended up passing the current modal (if any) through redux, and then conditionally rendering it inside a ReactCSSTransitionGroup. (Always rendering the ReactCSSTransitionGroup though.)
We're still only ever rendering one modal at a time, though it shouldn't be too hard to pass an array of modals through redux and render the topmost one, removing it when it is closed.

Ah okay, cool. Thanks for the update! I've been thinking of doing that, but I'm going to hold off and try to think up other alternatives, so that I can more easily pass in functions and non-serializable things into my modals (I'm making an effort to keep my store always serializable).


improve haproxy test

Test name
Status

haproxy
failed

big-data
failed

recreate
failed

scaling
passed

upgrade-haproxy
passed

scheduled-backup
passed

storage
passed

scaling-proxysql
passed

security-context
passed

self-healing
passed

self-healing-chaos
failed

upgrade-proxysql
passed

self-healing-advanced
passed

self-healing-advanced-chaos
failed

operator-self-healing
passed

operator-self-healing-chaos
failed

smart-update
passed

upgrade-consistency
passed

init-deploy
failed

restore-to-encrypted-cluster
failed

limits
failed

demand-backup
failed

commit: https://github.com/percona/percona-xtradb-cluster-operator/pull/1057/commits/c1324947e9b7ba263473ce8ea53323227d57ff8f
image: perconalab/percona-xtradb-cluster-operator:PR-1057-c1324947

Test name
Status

storage
passed

self-healing
passed

self-healing-chaos
failed

upgrade-haproxy
passed

recreate
passed

haproxy
failed

big-data
passed

scaling
passed

scheduled-backup
passed

scaling-proxysql
passed

security-context
passed

upgrade-proxysql
passed

smart-update
passed

upgrade-consistency
passed

self-healing-advanced
passed

self-healing-advanced-chaos
failed

restore-to-encrypted-cluster
passed

operator-self-healing
passed

operator-self-healing-chaos
failed

init-deploy
passed

demand-backup
passed

limits
passed

monitoring-2-0
passed

affinity
passed

one-pod
passed

pitr
passed

auto-tuning
passed

proxysql-sidecar-res-limits
passed

users
passed

demand-backup-encrypted-with-tls
passed

tls-issue-self
passed

tls-issue-cert-manager
passed

tls-issue-cert-manager-ref
passed

validation-hook
passed

commit: https://github.com/percona/percona-xtradb-cluster-operator/pull/1057/commits/c1324947e9b7ba263473ce8ea53323227d57ff8f
image: perconalab/percona-xtradb-cluster-operator:PR-1057-c1324947


Bug with more than 5 items

Hello, I am using you library and is working great now (Thanks) but I have to make a few changes to make it work.

First one was that the menu was render side wise not vertical. Maybe this is only with my display library. To fix it I swap the variables in render().

    def _render_cursor(self):
        for l in range(0, self.lines):
            self.lcd.move_to(0, l)

and

  def _render_options(self):
        # Render the options:
        for l, option in enumerate(self.viewport):
            self.lcd.move_to(1, l)  # Move to the line

2)When I have more than 8 options the view chunk retrun was off. With this code is working for me (also no need of import math anymore):
    def _current_chunk(self):
        return int((self.focus -1) / (self.lines ))  # current chunk

and finally I added the variable self.current with the menu object been render so I can control the sub menus more easily:

    def __init__(self, title, render_title=False):
        self.current = self

    def parent(self):
        if self.parent_menu:
            self.active = False
            self.parent_menu.current = self.parent_menu
            return self.parent_menu.start(self.lcd)

    def _choose_menu(self, submenu):
        self.active = False
        self.current = submenu

thanks for sharing the library. Regards.

Hey @facuver! Thanks for the extended feedback and suggestions! I haven't been able to have a look at this library for a while.. But I saw you used it for controllling your BIAB. Funny since that was my end-goal as well ;-)
I'll have a look at it during the holidays and will fix the issues you mentioned!


go: Add module

Added a very basic programs.go module. It can:

set GOPATH
set GOBIN
install the (possibly custom) go package
link packages into GOPATH

@rycee

@rvolosatovs Thanks! Rebased to master in 0635423e7365e9dbccac2fa154f53a45e952898e :+1:


[Enhancement] Add more test coverage for broker.slave.SlaveSynchronize

Before Creating the Enhancement Request

[X] I have confirmed that this should be classified as an enhancement rather than a bug/feature.

Summary
add more tests
Motivation
add more tests
Describe the Solution You'd Like
add more tests
Describe Alternatives You've Considered
add more tests
Additional Context
No response

link #8262


[7.0] Fix multi-column sort of collection. Fix #1237

Will try to test this as soon as I can. Thanks!

Released on v7.9.2 👍 thanks!


minimum gas should be scaled

Motivation
(Write your motivation for proposed changes here.)
Have you read the Contributing Guidelines on pull requests?
(Write your answer here.)
Test Plan
(Share your test plan here. If you changed code, please provide us with clear instructions for verifying that your changes work.)
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/diem/diem/developers.libra.org, and link to your PR here.)

https://github.com/diem/diem/issues/7238

https://github.com/diem/diem/issues/7238

Hi @guangyuz, thanks for reporting this and for the PR!
You bring up a really good point that we should probably decrease this to be more in-line with the average gas consumption. I don't think we want to necessarily scale the minimum gas amount using the gas_unit_scaling factor however; depending on the fluctuations of the gas scaling factor, and the min_transaction_gas_units this could lead to a minimum amount of zero being required for a non-zero gas price (which we definitely don't want).
It would probably be better to update the min_transaction_gas_units in both the DiemVMConfig and in the Rust code with a lower value (100) to be more in-line with the average gas consumption of a transaction.

Hi @guangyuz, thanks for reporting this and for the PR!
You bring up a really good point that we should probably decrease this to be more in-line with the average gas consumption. I don't think we want to necessarily scale the minimum gas amount using the gas_unit_scaling factor however; depending on the fluctuations of the gas scaling factor, and the min_transaction_gas_units this could lead to a minimum amount of zero being required for a non-zero gas price (which we definitely don't want).
It would probably be better to update the min_transaction_gas_units in both the DiemVMConfig and in the Rust code with a lower value (100) to be more in-line with the average gas consumption of a transaction.

It would probably be better to update the min_transaction_gas_units in both the DiemVMConfig and in the Rust code with a lower value (100) to be more in-line with the average gas consumption of a transaction.

Hi @tzakian,
I have some doubts, not sure if the correct value of "min_transaction_gas_units" is 100. I did some experiment to understand the logic behind.
I add log at charge_intrinsic_gas and charge_global_write_gas_usage to measure the gas used in each step. Then ran two stdlib scripts: add_currency_to_account and peer_to_peer_with_metadata and collected the following data:
add_currency_to_account:
default initial gas: GasUnits(4000000)
initial gas after scaling up: GasUnits(4000000000)
...step1.charge_intrinsic_gas: GasUnits(600)
...step2.charge_instr: GasUnits(89763)
...step3.charge_global_write_gas_usage: GasUnits(7200)
gas left: GasUnits(3999902437)
gas used(after scaling down): GasUnits(97)
peer_to_peer_with_metadata:
default initial gas: GasUnits(4000000)
initial gas after scaling up: GasUnits(4000000000)
...step1.charge_intrinsic_gas: GasUnits(600)
...step2.charge_instr: GasUnits(470541)
...step3.charge_global_write_gas_usage: GasUnits(14400)
gas left: GasUnits(3999514459)
gas used(after scaling down): GasUnits(485)
So, actually the intrinsic cost of the transactions is always less than 1 (after scaling down). If, like I did in the patch, we only guarantee that the submitted transactions max gas units covers the intrinsic cost of the transaction, then this will lead to a strange situation as you mentioned - minimum amount of zero being required for a non-zero gas price. But set "min_transaction_gas_units" to 100 is also somewhat inappropriate. We still can't run a script like add_currency_to_account although we have enough gas (97).
I can’t think of a better solution, maybe we should increase this "min_transaction_gas_units" value to greater than 1000？

It would probably be better to update the min_transaction_gas_units in both the DiemVMConfig and in the Rust code with a lower value (100) to be more in-line with the average gas consumption of a transaction.

Hi @tzakian,
I have some doubts, not sure if the correct value of "min_transaction_gas_units" is 100. I did some experiment to understand the logic behind.
I add log at charge_intrinsic_gas and charge_global_write_gas_usage to measure the gas used in each step. Then ran two stdlib scripts: add_currency_to_account and peer_to_peer_with_metadata and collected the following data:
add_currency_to_account:
default initial gas: GasUnits(4000000)
initial gas after scaling up: GasUnits(4000000000)
...step1.charge_intrinsic_gas: GasUnits(600)
...step2.charge_instr: GasUnits(89763)
...step3.charge_global_write_gas_usage: GasUnits(7200)
gas left: GasUnits(3999902437)
gas used(after scaling down): GasUnits(97)
peer_to_peer_with_metadata:
default initial gas: GasUnits(4000000)
initial gas after scaling up: GasUnits(4000000000)
...step1.charge_intrinsic_gas: GasUnits(600)
...step2.charge_instr: GasUnits(470541)
...step3.charge_global_write_gas_usage: GasUnits(14400)
gas left: GasUnits(3999514459)
gas used(after scaling down): GasUnits(485)
So, actually the intrinsic cost of the transactions is always less than 1 (after scaling down). If, like I did in the patch, we only guarantee that the submitted transactions max gas units covers the intrinsic cost of the transaction, then this will lead to a strange situation as you mentioned - minimum amount of zero being required for a non-zero gas price. But set "min_transaction_gas_units" to 100 is also somewhat inappropriate. We still can't run a script like add_currency_to_account although we have enough gas (97).
I can’t think of a better solution, maybe we should increase this "min_transaction_gas_units" value to greater than 1000？

Another idea is to scaling up max_gas_amount instead of scaling down min_txn_fee to avoid minimum amount of zero being required. See the new patch, this time the minimum amount 1 is required. ;-)

Another idea is to scaling up max_gas_amount instead of scaling down min_txn_fee to avoid minimum amount of zero being required. See the new patch, this time the minimum amount 1 is required. ;-)

@tzakian I disagree. It seems to me like we have 2 different kinds of gas measurements: scaled and unscaled. (It would be a good idea to introduce separate types to clarify when each should be used!) The VM operates on the scaled gas (* 1000) and the external world sees the unscaled gas (/ 1000). In this code in the adapter, we're comparing the unscaled max_gas_amount from the txn with the scaled result of calculate_intrinsic_gas(). That doesn't make sense to me, and I think @guangyuz had the right idea for fixing it in this PR. If we need to adjust gas constants to make that work, then so be it, but that part isn't clear to me.

@tzakian I disagree. It seems to me like we have 2 different kinds of gas measurements: scaled and unscaled. (It would be a good idea to introduce separate types to clarify when each should be used!) The VM operates on the scaled gas (* 1000) and the external world sees the unscaled gas (/ 1000). In this code in the adapter, we're comparing the unscaled max_gas_amount from the txn with the scaled result of calculate_intrinsic_gas(). That doesn't make sense to me, and I think @guangyuz had the right idea for fixing it in this PR. If we need to adjust gas constants to make that work, then so be it, but that part isn't clear to me.

After the discussion above, I agree that we want to scale the min/max gas constants.
I think the updated methodology of multiplying by the scaling factor as opposed to dividing by it makes sense to me, and I would be happy with this type of change, as long as it's applied consistently for both min and max gas constants.
This then leaves the question of what the min_gas_units and max_gas_units should be set at. I will take a look at the gas consumption across the scripts and reply back here on what seems to be best/data from that so we can determine a new min_gas_units. For the max_gas_units,  I think we should just scale the current set max_gas_units by the scaling factor amount.

After the discussion above, I agree that we want to scale the min/max gas constants.
I think the updated methodology of multiplying by the scaling factor as opposed to dividing by it makes sense to me, and I would be happy with this type of change, as long as it's applied consistently for both min and max gas constants.
This then leaves the question of what the min_gas_units and max_gas_units should be set at. I will take a look at the gas consumption across the scripts and reply back here on what seems to be best/data from that so we can determine a new min_gas_units. For the max_gas_units,  I think we should just scale the current set max_gas_units by the scaling factor amount.

I'm getting confused. I really think we should introduce separate types for scaled vs. unscaled gas units, because I'm having trouble keeping them straight. I hadn't looked at max_gas_units before. Unlike min_transaction_gas_units, max_gas_units is a field in the transaction, not in the gas schedule. Based on my explanation above, this should be using unscaled gas, because it is a value passed in from the outside world. However, that value is getting passed directly through to the VM in calls in execute_function without getting scaled. That seems like a bug to me. @tzakian do you agree?

I'm getting confused. I really think we should introduce separate types for scaled vs. unscaled gas units, because I'm having trouble keeping them straight. I hadn't looked at max_gas_units before. Unlike min_transaction_gas_units, max_gas_units is a field in the transaction, not in the gas schedule. Based on my explanation above, this should be using unscaled gas, because it is a value passed in from the outside world. However, that value is getting passed directly through to the VM in calls in execute_function without getting scaled. That seems like a bug to me. @tzakian do you agree?

@bob-wilson  We need to reconsider why scaled gas was introduced? Is it because of the precision of currency is too small? Then why not increase the precision of the currency?

@bob-wilson  We need to reconsider why scaled gas was introduced? Is it because of the precision of currency is too small? Then why not increase the precision of the currency?

@bob-wilson

I'm getting confused. I really think we should introduce separate types for scaled vs. unscaled gas units, because I'm having trouble keeping them straight.

Agreed! Pretty much all of the type-level mechanics are all there already around gas units, so this change shouldn't be too hard. In fact it's probably just doing a bit of refactoring to make the boundary and usage of the GasUnits type fit scaled vs unscaled.

I hadn't looked at max_gas_units before. Unlike min_transaction_gas_units, max_gas_units is a field in the transaction, not in the gas schedule. Based on my explanation above, this should be using unscaled gas, because it is a value passed in from the outside world. However, that value is getting passed directly through to the VM in calls in execute_function without getting scaled. That seems like a bug to me. @tzakian do you agree?

As it stands right now, I don't believe so. This is because:

All the fields in the user-submitted transaction are unscaled (and I think it should remain that way)
When they are being passed to the prologue functions they are also unscaled. This data is used to compute the max transaction fee liability for the transaction for balance checking purposes.
In the epilogues we pass in the (unscaled) remaining gas. See the implementation of remaining_gas for this. This unscaled number is used to compute the final transaction fee.

So in all of those cases that I saw in the diem_vm, we do want to be dealing with the unscaled amount of gas units.
Also I think there was confusion between  scaling the max_gas_units in the transaction, and the maximum_number_of_gas_units field in the gas constants struct, I meant the latter. I think the transaction gas should remain unscaled.
@jolestar

We need to reconsider why scaled gas was introduced? Is it because of the precision of currency is too small? Then why not increase the precision of the currency?

There are a couple reasons we decided on scaled gas vs increasing the currency precision:

If you make the currency precision smaller, you are reducing the largest amount of money that can be represented in that currency. We also probably don't want to have the representation of currency in the system dictated by gas, and instead give that flexibility to others (e.g., economists) so they can decide.
We can change the scaling factor as time goes on as, e.g., the instructions become less expensive computationally, whereas once a given precision is picked for a currency that can't really ever be changed. And in general, the scaling factor gives us a knob to turn to scale the transaction fees in the system as time goes on.

@bob-wilson

I'm getting confused. I really think we should introduce separate types for scaled vs. unscaled gas units, because I'm having trouble keeping them straight.

Agreed! Pretty much all of the type-level mechanics are all there already around gas units, so this change shouldn't be too hard. In fact it's probably just doing a bit of refactoring to make the boundary and usage of the GasUnits type fit scaled vs unscaled.

I hadn't looked at max_gas_units before. Unlike min_transaction_gas_units, max_gas_units is a field in the transaction, not in the gas schedule. Based on my explanation above, this should be using unscaled gas, because it is a value passed in from the outside world. However, that value is getting passed directly through to the VM in calls in execute_function without getting scaled. That seems like a bug to me. @tzakian do you agree?

As it stands right now, I don't believe so. This is because:

All the fields in the user-submitted transaction are unscaled (and I think it should remain that way)
When they are being passed to the prologue functions they are also unscaled. This data is used to compute the max transaction fee liability for the transaction for balance checking purposes.
In the epilogues we pass in the (unscaled) remaining gas. See the implementation of remaining_gas for this. This unscaled number is used to compute the final transaction fee.

So in all of those cases that I saw in the diem_vm, we do want to be dealing with the unscaled amount of gas units.
Also I think there was confusion between  scaling the max_gas_units in the transaction, and the maximum_number_of_gas_units field in the gas constants struct, I meant the latter. I think the transaction gas should remain unscaled.
@jolestar

We need to reconsider why scaled gas was introduced? Is it because of the precision of currency is too small? Then why not increase the precision of the currency?

There are a couple reasons we decided on scaled gas vs increasing the currency precision:

If you make the currency precision smaller, you are reducing the largest amount of money that can be represented in that currency. We also probably don't want to have the representation of currency in the system dictated by gas, and instead give that flexibility to others (e.g., economists) so they can decide.
We can change the scaling factor as time goes on as, e.g., the instructions become less expensive computationally, whereas once a given precision is picked for a currency that can't really ever be changed. And in general, the scaling factor gives us a knob to turn to scale the transaction fees in the system as time goes on.

So in all of those cases that I saw in the diem_vm, we do want to be dealing with the unscaled amount of gas units.

Agreed with all of those cases. The potential bug that concerns me is when the transaction's max_gas_amount is passed as an argument to execute_function. I think it needs to be scaled there. See, for example, the code in run_script_prologue, etc.

So in all of those cases that I saw in the diem_vm, we do want to be dealing with the unscaled amount of gas units.

Agreed with all of those cases. The potential bug that concerns me is when the transaction's max_gas_amount is passed as an argument to execute_function. I think it needs to be scaled there. See, for example, the code in run_script_prologue, etc.

So in all of those cases that I saw in the diem_vm, we do want to be dealing with the unscaled amount of gas units.

Agreed with all of those cases. The potential bug that concerns me is when the transaction's max_gas_amount is passed as an argument to execute_function. I think it needs to be scaled there. See, for example, the code in run_script_prologue, etc.

For clarity, I think we are both discussing the same code. Let me know if I'm wrong though. I would say the code in this example is fine. I say this because the transaction fee is calculated based on the unscaled gas usage in the epilogue, therefore we want to compute the maximum transaction fee here based off of the unscaled gas units passed in the transaction -- so we want to pass the unscaled value.

So in all of those cases that I saw in the diem_vm, we do want to be dealing with the unscaled amount of gas units.

Agreed with all of those cases. The potential bug that concerns me is when the transaction's max_gas_amount is passed as an argument to execute_function. I think it needs to be scaled there. See, for example, the code in run_script_prologue, etc.

For clarity, I think we are both discussing the same code. Let me know if I'm wrong though. I would say the code in this example is fine. I say this because the transaction fee is calculated based on the unscaled gas usage in the epilogue, therefore we want to compute the maximum transaction fee here based off of the unscaled gas units passed in the transaction -- so we want to pass the unscaled value.

Ah, I get it. Sorry for the confusion. I was reading that as a Rust argument to VM, but it is a Move argument to the prologue/epilogue function. I completely agree that this should be unscaled.

Ah, I get it. Sorry for the confusion. I was reading that as a Rust argument to VM, but it is a Move argument to the prologue/epilogue function. I completely agree that this should be unscaled.

@bob-wilson

I'm getting confused. I really think we should introduce separate types for scaled vs. unscaled gas units, because I'm having trouble keeping them straight.

Agreed! Pretty much all of the type-level mechanics are all there already around gas units, so this change shouldn't be too hard. In fact it's probably just doing a bit of refactoring to make the boundary and usage of the GasUnits type fit scaled vs unscaled.

The patch has been updated. ScaledGasUnits is introduced to describe gas units on the user side, and GasUnits is still used in the engine side.

@bob-wilson

I'm getting confused. I really think we should introduce separate types for scaled vs. unscaled gas units, because I'm having trouble keeping them straight.

Agreed! Pretty much all of the type-level mechanics are all there already around gas units, so this change shouldn't be too hard. In fact it's probably just doing a bit of refactoring to make the boundary and usage of the GasUnits type fit scaled vs unscaled.

The patch has been updated. ScaledGasUnits is introduced to describe gas units on the user side, and GasUnits is still used in the engine side.

By the way, @guangyuz, if you'd prefer to hand this off to someone else, feel free to ask. I really appreciate you pointing out this problem regardless. If you don't mind iterating on the PR, that is even better!

By the way, @guangyuz, if you'd prefer to hand this off to someone else, feel free to ask. I really appreciate you pointing out this problem regardless. If you don't mind iterating on the PR, that is even better!

This is along the general lines of what I was thinking, but after reviewing this, I would be more inclined to keep the "GasUnits" name for the externally visible amounts and use a new "InternalGasUnits" type for the amounts used within the VM. Otherwise, it's not very clear which way the scaling goes: are the "scaled" units the internal or external ones?

I agree with keeping the GasUnits name as-is. I was thinking that we wouldn't even have a newtype for non-scaled gas units (these would just be u64s), that way there's no confusion, and since metering should never been done against anything other than a GasUnit type. But I'm open to having a newtype for unscaled gas units as well.

It really seems to me like the maximum_number_of_gas_units gas constant should be expressed in the "external" units, especially when I see code like the get_transaction_parameters function in language/testing-infra/functional-tests/src/evaluator.rs. But, changing that might be more trouble than it's worth, since it is stored on-chain in the gas schedule. @tzakian WDYT? If we don't change it, the new type should help avoid mistakes.

I think we should scale this if we are scaling the min_transaction_gas_units. I think we could keep this as-is as and just treat it as scaled now, since it's already set quite high and I don't think dividing it by, e.g., 1000 will cause any issues.
To be precise my thoughts are:

Any representation of "units of gas" in the gas constants stored on-chain should be defined in a scaled form (i.e., both min_transaction_gas_units and maximum_number_of_gas_units.
Any representation of units of gas in the transaction submitted should be unscaled.

The main thing for me is consistency; either the struct constants need to all be scaled, or all be unscaled.

It also looks like the calculate_intrinsic_gas function might be better returning "external" units. Besides the comparison that is adjusted in diem_vm.rs, see the code using that in language/testing-infra/e2e-tests/src/account_universe/bad_transaction.rs.

At least from a preliminary look this doesn't look to be a problem to me, but I may be missing something..

By the way, @guangyuz, if you'd prefer to hand this off to someone else, feel free to ask. I really appreciate you pointing out this problem regardless. If you don't mind iterating on the PR, that is even better!

I also want to second this -- and wanted to also thanks as well for bringing this up! Let us know how you want to proceed.

This is along the general lines of what I was thinking, but after reviewing this, I would be more inclined to keep the "GasUnits" name for the externally visible amounts and use a new "InternalGasUnits" type for the amounts used within the VM. Otherwise, it's not very clear which way the scaling goes: are the "scaled" units the internal or external ones?

I agree with keeping the GasUnits name as-is. I was thinking that we wouldn't even have a newtype for non-scaled gas units (these would just be u64s), that way there's no confusion, and since metering should never been done against anything other than a GasUnit type. But I'm open to having a newtype for unscaled gas units as well.

It really seems to me like the maximum_number_of_gas_units gas constant should be expressed in the "external" units, especially when I see code like the get_transaction_parameters function in language/testing-infra/functional-tests/src/evaluator.rs. But, changing that might be more trouble than it's worth, since it is stored on-chain in the gas schedule. @tzakian WDYT? If we don't change it, the new type should help avoid mistakes.

I think we should scale this if we are scaling the min_transaction_gas_units. I think we could keep this as-is as and just treat it as scaled now, since it's already set quite high and I don't think dividing it by, e.g., 1000 will cause any issues.
To be precise my thoughts are:

Any representation of "units of gas" in the gas constants stored on-chain should be defined in a scaled form (i.e., both min_transaction_gas_units and maximum_number_of_gas_units.
Any representation of units of gas in the transaction submitted should be unscaled.

The main thing for me is consistency; either the struct constants need to all be scaled, or all be unscaled.

It also looks like the calculate_intrinsic_gas function might be better returning "external" units. Besides the comparison that is adjusted in diem_vm.rs, see the code using that in language/testing-infra/e2e-tests/src/account_universe/bad_transaction.rs.

At least from a preliminary look this doesn't look to be a problem to me, but I may be missing something..

By the way, @guangyuz, if you'd prefer to hand this off to someone else, feel free to ask. I really appreciate you pointing out this problem regardless. If you don't mind iterating on the PR, that is even better!

I also want to second this -- and wanted to also thanks as well for bringing this up! Let us know how you want to proceed.

@tzakian and I just chatted.

This is along the general lines of what I was thinking, but after reviewing this, I would be more inclined to keep the "GasUnits" name for the externally visible amounts and use a new "InternalGasUnits" type for the amounts used within the VM. Otherwise, it's not very clear which way the scaling goes: are the "scaled" units the internal or external ones?

I agree with keeping the GasUnits name as-is. I was thinking that we wouldn't even have a newtype for non-scaled gas units (these would just be u64s), that way there's no confusion, and since metering should never been done against anything other than a GasUnit type. I'm open to having a newtype for unscaled gas units as well, just I'm not sure what that buys us is worth the extra complexity.

I think I convinced Tim about the need for a newtype. As we were talking, I found it really tough to keep straight between the "scaled" and "unscaled" terminology, so at least for me, "internal" (e.g., InternalGasUnits) and "external" (e.g., GasUnits) works a lot better.

It really seems to me like the maximum_number_of_gas_units gas constant should be expressed in the "external" units, especially when I see code like the get_transaction_parameters function in language/testing-infra/functional-tests/src/evaluator.rs. But, changing that might be more trouble than it's worth, since it is stored on-chain in the gas schedule. @tzakian WDYT? If we don't change it, the new type should help avoid mistakes.

I think we should scale this if we are scaling the min_transaction_gas_units. I think we could keep this as-is as and just treat it as scaled now, since it's already set quite high and I don't think dividing it by, e.g., 1000 will cause any issues.

One thing that Tim and I realized is that changing the on-chain gas schedule or the interpretation of the existing values will be difficult. We can do it if we have to, but it will add a lot of complexity since we need the ability to re-execute old transactions and get identical results -- including for gas fees. One thing we can do without too much complexity is to fix this bug where transactions are being incorrectly discarded during validation, as long as the fix doesn't change the behavior for transactions that were previously treated as valid.

To be precise my thoughts are:

Any representation of "units of gas" in the gas constants stored on-chain should be defined in a scaled form (i.e., both min_transaction_gas_units and maximum_number_of_gas_units.
Any representation of units of gas in the transaction submitted should be unscaled.

The main thing for me is consistency; either the struct constants need to all be scaled, or all be unscaled.

Nope. See above. What we need is to classify the existing constants as either internal or external units. Here's my take on that:
Internal

global_memory_per_byte_cost
global_memory_per_byte_write_cost
min_transaction_gas_units (note inconsistency where this is used in e2e-testsuite/src/tests/verify_txn.rs)
intrinsic_gas_per_byte

External

maximum_number_of_gas_units (i.e., the conversion in the current PR is unnecessary)

It also looks like the calculate_intrinsic_gas function might be better returning "external" units. Besides the comparison that is adjusted in diem_vm.rs, see the code using that in language/testing-infra/e2e-tests/src/account_universe/bad_transaction.rs.

At least from a preliminary look this doesn't look to be a problem to me, but I may be missing something..

Tim pointed out the root problem with the calculate_intrinsic_gas function: we're using the result in one place as "external" units and in another place as "internal" units. We should leave the function returning internal units and convert when comparing against the transaction's max_gas_amount (as @guangyuz proposed in this PR).

@tzakian and I just chatted.

This is along the general lines of what I was thinking, but after reviewing this, I would be more inclined to keep the "GasUnits" name for the externally visible amounts and use a new "InternalGasUnits" type for the amounts used within the VM. Otherwise, it's not very clear which way the scaling goes: are the "scaled" units the internal or external ones?

I agree with keeping the GasUnits name as-is. I was thinking that we wouldn't even have a newtype for non-scaled gas units (these would just be u64s), that way there's no confusion, and since metering should never been done against anything other than a GasUnit type. I'm open to having a newtype for unscaled gas units as well, just I'm not sure what that buys us is worth the extra complexity.

I think I convinced Tim about the need for a newtype. As we were talking, I found it really tough to keep straight between the "scaled" and "unscaled" terminology, so at least for me, "internal" (e.g., InternalGasUnits) and "external" (e.g., GasUnits) works a lot better.

It really seems to me like the maximum_number_of_gas_units gas constant should be expressed in the "external" units, especially when I see code like the get_transaction_parameters function in language/testing-infra/functional-tests/src/evaluator.rs. But, changing that might be more trouble than it's worth, since it is stored on-chain in the gas schedule. @tzakian WDYT? If we don't change it, the new type should help avoid mistakes.

I think we should scale this if we are scaling the min_transaction_gas_units. I think we could keep this as-is as and just treat it as scaled now, since it's already set quite high and I don't think dividing it by, e.g., 1000 will cause any issues.

One thing that Tim and I realized is that changing the on-chain gas schedule or the interpretation of the existing values will be difficult. We can do it if we have to, but it will add a lot of complexity since we need the ability to re-execute old transactions and get identical results -- including for gas fees. One thing we can do without too much complexity is to fix this bug where transactions are being incorrectly discarded during validation, as long as the fix doesn't change the behavior for transactions that were previously treated as valid.

To be precise my thoughts are:

Any representation of "units of gas" in the gas constants stored on-chain should be defined in a scaled form (i.e., both min_transaction_gas_units and maximum_number_of_gas_units.
Any representation of units of gas in the transaction submitted should be unscaled.

The main thing for me is consistency; either the struct constants need to all be scaled, or all be unscaled.

Nope. See above. What we need is to classify the existing constants as either internal or external units. Here's my take on that:
Internal

global_memory_per_byte_cost
global_memory_per_byte_write_cost
min_transaction_gas_units (note inconsistency where this is used in e2e-testsuite/src/tests/verify_txn.rs)
intrinsic_gas_per_byte

External

maximum_number_of_gas_units (i.e., the conversion in the current PR is unnecessary)

It also looks like the calculate_intrinsic_gas function might be better returning "external" units. Besides the comparison that is adjusted in diem_vm.rs, see the code using that in language/testing-infra/e2e-tests/src/account_universe/bad_transaction.rs.

At least from a preliminary look this doesn't look to be a problem to me, but I may be missing something..

Tim pointed out the root problem with the calculate_intrinsic_gas function: we're using the result in one place as "external" units and in another place as "internal" units. We should leave the function returning internal units and convert when comparing against the transaction's max_gas_amount (as @guangyuz proposed in this PR).

@tzakian and I just chatted.

This is along the general lines of what I was thinking, but after reviewing this, I would be more inclined to keep the "GasUnits" name for the externally visible amounts and use a new "InternalGasUnits" type for the amounts used within the VM. Otherwise, it's not very clear which way the scaling goes: are the "scaled" units the internal or external ones?

I agree with keeping the GasUnits name as-is. I was thinking that we wouldn't even have a newtype for non-scaled gas units (these would just be u64s), that way there's no confusion, and since metering should never been done against anything other than a GasUnit type. I'm open to having a newtype for unscaled gas units as well, just I'm not sure what that buys us is worth the extra complexity.

I think I convinced Tim about the need for a newtype. As we were talking, I found it really tough to keep straight between the "scaled" and "unscaled" terminology, so at least for me, "internal" (e.g., InternalGasUnits) and "external" (e.g., GasUnits) works a lot better.

I am also in favor of using GasUnits / InternalGasUnits, which allows us not to change the TransactionMetadata type.

@tzakian and I just chatted.

This is along the general lines of what I was thinking, but after reviewing this, I would be more inclined to keep the "GasUnits" name for the externally visible amounts and use a new "InternalGasUnits" type for the amounts used within the VM. Otherwise, it's not very clear which way the scaling goes: are the "scaled" units the internal or external ones?

I agree with keeping the GasUnits name as-is. I was thinking that we wouldn't even have a newtype for non-scaled gas units (these would just be u64s), that way there's no confusion, and since metering should never been done against anything other than a GasUnit type. I'm open to having a newtype for unscaled gas units as well, just I'm not sure what that buys us is worth the extra complexity.

I think I convinced Tim about the need for a newtype. As we were talking, I found it really tough to keep straight between the "scaled" and "unscaled" terminology, so at least for me, "internal" (e.g., InternalGasUnits) and "external" (e.g., GasUnits) works a lot better.

I am also in favor of using GasUnits / InternalGasUnits, which allows us not to change the TransactionMetadata type.

By the way, @guangyuz, if you'd prefer to hand this off to someone else, feel free to ask. I really appreciate you pointing out this problem regardless. If you don't mind iterating on the PR, that is even better!

This is an incompatible modification. If you are willing to take over, it is undoubtedly the best.

By the way, @guangyuz, if you'd prefer to hand this off to someone else, feel free to ask. I really appreciate you pointing out this problem regardless. If you don't mind iterating on the PR, that is even better!

This is an incompatible modification. If you are willing to take over, it is undoubtedly the best.

If you are willing to take over, it is undoubtedly the best.

Sure, I'll volunteer, since I already spent a bunch of time digging into it yesterday. Thanks again, @guangyuz !!

If you are willing to take over, it is undoubtedly the best.

Sure, I'll volunteer, since I already spent a bunch of time digging into it yesterday. Thanks again, @guangyuz !!


Some Turbomole fixes

Description

This PR incorporates some fixes/additions for the Turbomole runner:

Pass SCF convergence and SCF max. iterations to define
Correctly enable OMP
Pass environment to execute

@loriab I'm actually @maxscheurer but from my corporate GH account 😄
Questions

[ ] Is the new test okay using xfail?
[ ] How should one allow a user-specified scratch directory? Is TaskConfig -> execute the correct route?
[ ] I changed the (unusual?) jobs_per_node default from 2 to 1, is that okay, or was there a reason for it being set to 2 specifically?
[ ] make format reformats almost all files...

Changelog description

Status

[ ] Code base linted
[ ] Ready to go

Is the new test okay using xfail?

sure, that's fine

How should one allow a user-specified scratch directory? Is TaskConfig -> execute the correct route?

Yes, that's it. User-side here: https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/tests/test_canonical_config.py#L160-L161 . Code side here (first and last lines): https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/cfour/runner.py#L85-L154 . The former is a something of a standard test you could add turbomole to sometime.

I changed the (unusual?) jobs_per_node default from 2 to 1, is that okay, or was there a reason for it being set to 2 specifically?

I'm leery of that change. Was it causing trouble as 2? I don't know why it's not 1 (admittedly more usual) -- maybe 2 avoided only serial testing?

make format reformats almost all files...

I've squashed that now :-)
Anyways, all lgtm. After 414 is in and tests can pass, I can adapt this PR. Main question is jobs_per_node, I guess. Thanks!

I'm leery of that change. Was it causing trouble as 2? I don't know why it's not 1 (admittedly more usual) -- maybe 2 avoided only serial testing?

IIRC if you don't specify anything, then this divides the number of cores for the job by 2, which I think is a bit unexpected. 😄

The jobs_per_node traces back to here https://github.com/dgasmith/QCEngine/commit/41e7da039102195a5f17e5526a485f5cdefc6bff . I still don't see why it's 2, unless just to have something multiprocess to test against.
Changing to 1 causes a gamess test to fail, but not for a good reason. Still investigating ...

Ok, see #415. I think the jobs_per_node should be reset to 1, but not in isolation. Do you agree? If so, you or I can remove it, then we'll get this one in, and maybe openmm will cooperate in CI.


BiDI and RTL support

For newsboat 2.13.0 in debian: Hebrew (and I guess this would be true also for Arabic, Farsi, and other LTR [edit: should be RTL] languages) feeds display their headlines and content in reverse, that is, both on the article listing screen and for the internal pager.  I suppose part of a work-around would be to use an external pager, but that wouldn't help for the article listing screen, would it.

Hi! Can you provide a URL of a feed I can use for testing this? Not knowing any of LTR languages, it's not that easy to find one and figure out if it's displayed correctly.

Sure. Here's an example set from a major Israeli newspaper:
https://www.haaretz.co.il/misc/rss
https://www.haaretz.co.il/cmlink/1.1472481 ~HaAretz_-_Op-Ed
https://www.haaretz.co.il/cmlink/1.1472228 ~HaAretz_-_Policy_and_Security
https://www.haaretz.co.il/cmlink/1.1472258 ~HaAretz_-_Science_and_Environment
https://www.haaretz.co.il/cmlink/1.1471359 ~HaAretz_-_Law_and_Crime
https://www.haaretz.co.il/cmlink/1.1410478 ~HaAretz_-_News
https://www.haaretz.co.il/cmlink/1.1471322 ~HaAretz_-_International
I was surprised that I wasn't able to get a good link for an Al-Jazeera Arabic RSS feed; I hope it isn't being censored here in the USA, but there is a CNN feed page:
http://archive.arabic.cnn.com/rss/
If you don't read either language, you can exploit a common feature of both Hebrew and Arabic - some of their letters look different when they appear at the end of a word, so if you see those glyphs on the left side of a word, that's a sign that the bidi/RTL is working. In Hebrew, there are only five of those glyphs: ך ם ן ף ץ. Arabic is tougher to give you example for, because their letters adopt even more variations for when they're in the middle of a word or standing alone, and modern computers are too smart, so when I type an end-glyph as a stand-alone character, it auto-converts to the stand-alone glyph (usually that's a welcome convenience feature, but it's frustrating in this case)

Oops. I got my directionality mixed up. Hebrew and Arabic are RTL (right-to-left), as opposed to English which is LTR (left-to-right).

Thanks for the links, and also for that note about five glyphs that appear differently—really handy! I reproduced this bug with 86097d60a143dd87e700586b2609a679ba7082dc. Comparing with Firefox 60.6, I see that Newsboat does show text in different order.
haaretz.co.il feeds have an empty language tag, so I don't know what I'll be able to do there. Firefox does detect the language somehow, though.
CNN feeds do have the tag set, and we do parse it, but then… I can't find any code that actually uses this information.
To make matters even worse, it appears I can't just reverse the string to implement RTL: for example, numbers should always appear LTR. I'll get back to you once I studied this stuff a bit more :)

From my vague memory of long-past experience, here are some factoids
that might be helpful:

I think unicode has intrinsic bidi (bi-directional) support in the
form of special non-printable 'direction' characters, so if the
codebase uses, or can use, a modern unicode text handling library,
the job should simple and not require specific language detection.

I know of two command-line tools to handle bidi:
2.1) bidiv is a debian package, based upon..
2.2) fribidi. This is packaged by debian along with a library, so it
might be useful for you. In debian, a package search yields ...
 $ apt-cache search fribidi
 libfribidi-bin - Free Implementation Unicode BiDi algorithm 2.1
 libfribidi-dev - Development files for FreeBidi library
 libfribidi0 - Free Implementation of the Unicode BiDi algorithm
 libtext-bidi-perl - Unicode bidi algorithm Perl using libfribidi
 python-pyfribidi - FriBidi Python bindings
 python-pyfribidi-dbg - FriBidi Python bindings (debug symbols)
 libraqm0 - Library for complex text layout

On 2019-04-09 11:46, Alexander Batischev wrote:

...
Firefox does detect the language somehow, though.

Debian shows three bidi packages for rust, so my guess is that's how
firefox deals with it.
 $ apt-cache search bidi | grep rust
 librust-unicode-bidi+serde-dev - The Unicode Bidirectional Algorithm -
 librust-unicode-bidi+with-serde-dev - The Unicode Bidirectional
 librust-unicode-bidi-dev - The Unicode Bidirectional Algorithm - Rust

To make matters even worse, it appears I can't just reverse the string
to implement RTL: for example, numbers should always appear LTR.

Heh heh heh (insert emoji for evil laughter). There's more. The bidi
algorithm needs to handle changes of direction within sentences for
embedded LTR strings (eg. כמו זה), and properly detect a paragraph's
primary direction for justification and indentation. You would be
badly advised to 'roll your own' on this one if it's at all avoidable;
bidi has been a pain for decades.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Any progress on this issue?

Still haven't studied how to properly handle RTL, sorry :( @Boruch-Baum gave some excellent pointers, though, so if anyone wants to step in, I'm happy to help navigate around the Newsboat code.

The text seems to look pretty good in an RTL-enabled terminal like WezTerm, or Terminator (probably all vte3 terminals including gnome-terminal), but as I am unable to read those languages I do not know if something has to be improved in newsboat.

@progandy
I test that, and it shows in the wrong direction. I don't set any specific config for config in Newsboat or Wezterm if it needed.
As an example, if Newsboat should show book, it is show that like this koob
show alphabet in wrong order


Widgets example panics when adding new element to list

Describe the bug
Widgets example panics when adding new element to list
To Reproduce
Steps to reproduce the behavior:

git clone https://github.com/redox-os/orbtk
cd orbtk
cargo run --example widgets
Click on '+' button in Items section

Expected behavior
Example showcasing that widgets are working should not panic.
Screenshots
thread 'main' panicked at 'Widget with name: ItemsWidget and entity: 102 does not contain property width type_id TypeId { t:<PHONE_NUMBER>559642870 } for key: blub', /home/michal/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/macros.rs:16:9

Desktop:

OS: Fedora Remix for WSL (Windows 10 v1909)
Version: 0.3.1-alpha3

Additional context
It seems that line 44 in examples/widgets.rs
ctx.child("items").set::<usize>("blub", len + 1);

is causing the panic

fixed


[Bug]: Integration to security hub

Currently I cannot fined Prowler in the security hub integrations. IS there any workaround what is not documented. I tested also the cli integration and received following message:
aws securityhub enable-import-findings-for-product --region us-east-1 --product-arn arn:aws:securityhub:us-east-1::product/prowler/prowler

An error occurred (ResourceNotFoundException) when calling the EnableImportFindingsForProduct operation: {"productArn":"arn:aws:securityhub:us-east-1::product/prowler/prowler"}

Already Talked to AWS as it looks like the problem is on AWS side.

Please keep us posted with whatever the issue is. And thanks for let us know @sottlmarek!

Currently testing it. I spoke with AWS guys in Community builders slack to report it. Yesterday was not possible to approve ASFF ingestion in AWS Security hub integrations. Now, I am testing if it works for more regions.

I have done a test in us-east-1 and worked properly for me.

Now it works, yesterday not. When I searched prowler in integrations it was not there. AWS folks confirmed that bug. Now it looks good.

Sounds great!! Thank you @sottlmarek


Blender crashes when launching interactive render

I at first thought this was when any time that the preview material is render, but it seems to crash when you first load blender and the materials are initializing and the previews are rendering. i have noticed though if I wait till the preview shows up then render it doesn't crash.
I will try to get a sample scene where it does it reliably to give more detail.

Have you synced the latest updates?  Should have fixed IPR crashing when preview material is open

Looking at change log I have the latest as of 7/31 that contains the merge from the pull request as well.
I'm using Blender 2.75a and running mac os x 10.10.4 -- it seems to only be when I load a scene and seems like it goes through and loads the materials, actually I need to check but it was a cycles scene at one time so perhaps the conversion is in process not sure, but have saved it since I switched Renderman.
I could not reproduce it by adding a new material and hit interactive render while the preview is blank, it seems to work with out crashing that way. I will see if I can't provide a scene that does it reliably. Don't mean to be as vague, I know it will help to see it.

https://drive.google.com/file/d/0B1GplrdFdoo-TTlROG5oZ1BEem8/view?usp=sharing
Took the default scene, simply changed the default render and saved it. Click new scene, so resets to the default scene, open the file and clicked interactive render and it crashes. Note that the preview section is blank, no black box or anything, then once it turns black and renders I can click interactive render and it works no problem.

I think this is somewhat related (or the fix will be) to the switching scene while IPR is running.  Basically we need to remove the menu item, I guess, if the render engine isn't prman.  And do some cleanup if changing files if ipr is running.

Fixed.


hold the burger better

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
Describe the solution you'd like
A clear and concise description of what you want to happen.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.

I'm assuming you want to move an item's location so it isn't in the players center of mass (The description wasn't filled out when this was created)

I'm assuming you want to move an item's location so it isn't in the players center of mass (The description wasn't filled out when this was created)


View preview of guide content in link cards

I WANT to be able to see a preview of the guide content
SO that I can know if that content is relevant to me
GIVEN I am on the learn page
WHEN I scroll down to the Guides section
THEN I can see each guide with name and preview content

We've redesigned the website completely and this issue does not apply anymore.


[FLINK-35158][runtime] Error handling in StateFuture's callback

What is the purpose of the change
This PR implements the error handling in StateFuture's callback, making the job fail when any exception exception occurs in StateFuture's callback.
Brief change log

Add exception handling to the implementation class of StateFuture/
Wire the exception in StateFuture's callback to environment.failExternally.

Verifying this change
This change added/updated tests and can be verified as follows:

AsyncExecutionControllerTest#testException
ContextStateFutureImplTest

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): ( no )
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? ( no)
If yes, how is the feature documented? (JavaDocs)

Thanks for the detailed review, rebased to master.  Will merge after CI green.


honor lnbits payments in check_invoice_status #336

Hope to get this merged so that TPoS and Charges work as expected for internal payments.

Would this not mean failed payments clear as paid?

@arcbtc from what i can see, please double check it, it only gets set as not pending if it's paid.
@property
    def pending(self) -> bool:
        return self.paid is not True

Thank you for the feedback. I understood it the same way @talvasconcelos does. Happy to help/improve.


python3Packages.jsonstreams: Init at 0.5.0

Motivation for this change
Make this python library available.
Things done

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

[x] NixOS
[ ] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[x] Tested execution of all binary files (usually in ./result/bin/)
[x] Determined the impact on package closure size (by running nix path-info -S before and after)
0 → 110,903,680
[x] Ensured that relevant documentation is up to date
[x] Fits CONTRIBUTING.md.


Get PR for SPARK-17059 merged upstream (file-based partition pruning)

https://issues.apache.org/jira/browse/SPARK-17059

doable with dsv2


JSON decode error fix

Outlined in #255 there can be JSON decode errors when running TheHive4Py on AWS Lambda most likely due to Lambda not including the correct package that contains JSONDecodeError.
This PR updates the JSONDecodeError to come from the requests package. Thank you to all the hard work from @octocolby for identifying the root cause of this issue and providing a fix.

I've made a fix for this one before, but it seems like the PR got lost in the meantime.
Did you try this fix against simplejson?
In case it doesn't work with other libs please fall back using this snippet:
import json as jsonlib

@Kamforka No we didn't try against simplejson however, requests will decide if there is simplejson or json so wouldn't that be ideal as you don't have to worry about the lib it will determine it for you?
I will add my Python knowledge isn't as good as the people here so i will also defer to @octocolby if he has any other thoughts.
https://github.com/psf/requests/blob/bda7f0171f8bba17989d3a2c28dfa9a9261b1b65/requests/compat.py#L39

@wizedkyle I'm not sure requests will choose the right decode error as there is a thread around this issue for years now: https://github.com/psf/requests/issues/4842
I don't mind your current solution if you can verify it against simplejson, I'd do that myself but for the time being I don't have access to my workspace, I can check it earliest end of this week.

@Kamforka I am going to test the fix against simplejson tonight and will let you know the results.

@wizedkyle Maybe I explained myself poorly. I didn't mean to use the simplejson inside thehive4py, what I meant is that your original fix should be tested against an environment where simplejson is present, i.e. pip install simplejson thehive4py in your testing environment.
But no worries now I also have to verify it on my own, I'll get back to you.

Okay I've checked the requests way of doing things and I'm not sure why it worked for you but I immediately get this error:
>>> from requests import JSONDecodeError
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'JSONDecodeError' from 'requests'

So I'd just suggest to fall back to my original recommendation and use the standard json library to parse the content of the response:
json_data = jsonlib.loads(response.content)

May I ask you to add this change to the PR? You need to adjust _process_text_response and _process_error_response for this.
I also tested it in an environment with and without simplejson and it worked fine.

Fixing this in #259


Implemented bulk read/write with BatchGet/Write APIs

Fixes #114

@vinaykumarchella I no longer special case throttling and ISE here.

Can you rebase this change.

Rebased.


Output channel doesn't exist with gitlens.outputLevel default

I tried to apply a stash, it failed.
A notification told me to find more details in the output channel.
I clicked "Open output channel", nothing happened

I had to read the source to find that the output channel isn't created when the "output level" setting is silent, which is the default value. It seems like "errors" would be a more useful default.

@roblourens if this was helpful to you, consider giving a reward to the issue solver using Speedoss.com


Error fetching submodules

Hello! I am unble to build pyeos on my system (Ubuntu 14.04) because of this error when fetching submodules. Do you have any fix maybe?

Fixed, thanks.