Refactor OTLP Metrics Exporters

Currently, we have OTLP trace exporters to be implemented in an inherited class pattern:
OTLPTraceExporter (grpc) -> OTLPGRPCExporterNodeBase -> OTLPExporterBase
OTLPTraceExporter (HTTP)-> OTLPExporterNodeBase -> OTLPExporterBase
OTLPTraceExporter (proto) -> OTLPProtoExporterNodeBase -> OTLPExporterNodeBase -> OTLPExporterBase

On the other hand, the OTLP metric exporters are not in this pattern. The base OTLP metric exporter, a MetricExporter, has an internal metric exporter, which also implements the MetricExporter interface. This pattern looks abstruse to me.
Can the OTLP metric exporter be implemented without the intermediate exporter?
/cc @pichlermarc

Yes implementing this way should be possible. I originally introduced this as to not bloat up the PRs (#2871, #2893), as they were already way too large at that time.
The reason for that was that the OTLP Exporter expects a list of objects passed to the export() function. This worked for the Trace exporters as they are expecting a list of spans, but not for the Metrics Exporters, that pass a single ResourceMetrics instance. My solution for that was to introduce this Proxy, which kept the diff manageable, but looks, as you already said, abstruse and is far from the optimal solution. :slightly_frowning_face:
My original intention was always to follow up with more refactoring PRs to simplify the exporters (and their tests), but I unfortunately never came around to doing it. If needed, I can prioritize this. :slightly_smiling_face:

My original intention was always to follow up with more refactoring PRs to simplify the exporters (and their tests), but I unfortunately never came around to doing it. If needed, I can prioritize this. 🙂

No, not in a hurry as it is working now. I was opening this issue to see if this is intentional and if not we can track the progress here. Thank you for the clarification.

With #5031 this now becomes a bit more actionable. I'll refine this issue once the PR is merged to reflect what needs to be done in a bit more detail. 🙂

Actually, the leftovers from this can be handled in one simple follow-up.
#5031 removes the last of these proxy implementations. Once it is merged, I'll follow up with another PR to removing the array-wrapping inside the exporters, as that can be handled by the json/proto serializers (away from the public API) now.

Assigning myself since #5159 will fix this 🙂


⚠ SkipTheTrailers has degraded performance

In 20bd714, SkipTheTrailers ($STT) experienced degraded performance:

HTTP code: 200
Response time: 1636 ms

Resolved: SkipTheTrailers performance has improved in 6d2f2b0.


Dependencies show non existing connection

What kind of issue is this?

[x] Bug report. If you’ve found a bug, spend the time to write a failing test. Bugs with tests
get fixed and stay fixed. If you have a solution in mind, skip raising an issue and open a
pull request instead.

In a demo environment I have 4 Node.js services.
Node.js is version 8.
The service methods look like that
// express-frontend
router.get('/', async (req, res, next) => {
  try {
    const httpres = await axios.get('http://localhost:3001'); // == service-gateway
    res.render('index', { title: 'Express' });
  } catch(err) {
    return next(err);
  }
});

// service-gateway
let toggle = false;

router.get('/', async (req, res, next) => {
  try {
    if(!toggle) {
      await axios.get('http://localhost:3010'); // service-green
      toggle = true;
    } else {
      await axios.get('http://localhost:3020'); // service-blue
      toggle = false;
    }
    res.render('index', { title: 'Express' });
  } catch (err) {
    console.log(err);
    return res.status(500).end();
  }
});

service-blue and service-green are simple express apps with one index route returning 200.
So it's clear that we have a graph in the form A-B=C or D

Which would be A-B-C and A-D
If I use superagent and regular callbacks, the graph looks correct.
Looks as if there was a context problem with two consecutive http calls in conjunction with await.

This is a good issue description, perhaps on the wrong project. I think you might be asking about zipkin-js.
Can you re-issue this there, and also include the example code and/or example json associated with the trace. Context issues are subtle so the setup is something likely to be helpful, hence the request for example code or failing unit test.

I will.


[FEA] [AUDIT] Support expressions to work with collated strings

Is your feature request related to a problem? Please describe.
This is an umbrella to list all the Spark commits related to supporting collated strings

[ ] [SPARK-47413][SQL] add support to substr/left/right for collations  https://github.com/apache/spark/commit/e1432ef6405
[ ] [SPARK-47411][SQL] Support StringInstr & FindInSet functions to work with collated strings https://github.com/apache/spark/commit/256fc51508e
[ ] [SPARK-47412][SQL] Add Collation Support for LPad/RPad https://github.com/apache/spark/commit/885e98ecbe6
[ ] [SPARK-47873][SQL] Write collated strings to Hive metastore using the regular string type https://github.com/apache/spark/commit/9d9f292ac94
[ ] [SPARK-47414][SQL] Lowercase collation support for regexp expressions https://github.com/apache/spark/commit/b4624bf4be2
[ ] [SPARK-47350][SQL] Add collation support for SplitPart string expression https://github.com/apache/spark/commit/6c40214f3d9
[ ] [SPARK-47351][SQL] Add collation support for StringToMap & Mask string expressions https://github.com/apache/spark/commit/afe310d617e
[ ] [SPARK-47476][SQL] Support REPLACE function to work with collated strings https://github.com/apache/spark/commit/07b84dd57e3
[ ] [SPARK-47567][SQL] Support LOCATE function to work with collated strings https://github.com/apache/spark/commit/7b1147a05a6
[ ] [SPARK-48003][SQL] Add collation support for hll sketch aggregate https://github.com/apache/spark/commit/3aea6c258bf
[ ] [SPARK-47352][SQL] Fix Upper, Lower, InitCap collation awareness https://github.com/apache/spark/commit/b9f2270f5b0

The plan is explicitly to not support collated strings and to fallback to the CPU if we see them. This is a very large amount of work to try and support it, and I don't think we want to try and tackle it would a clear customer need.


Build failures with GHC < 8

See https://matrix.hackage.haskell.org/#/package/vector-th-unbox
For 7.10.3, the error is
Data/Vector/Unboxed/Deriving.hs:4:14:
    Unsupported extension: TemplateHaskellQuotes

Due to https://github.com/tsurucapital/vector-th-unbox/commit/a06a865a5a7d950ea5954b4b51ca6c96a9d94c46, I don't think this support is coming back.

I don't really have an opinion on which GHC versions this package supports. I just encountered this issue in a package that depends on vector-th-unbox via criterion (via statistics).
In any case, could you tighten the lower bound on base for v0.2.2 with a Hackage revision?

Sounds like a good idea, will do.

Thanks for the revision! :)

FYI, you can add other-extensions: TemplateHaskellQuotes (in revision!) and it would had similar effect, but were semantically more correct.

@sjakobi as you are a Hackage Trustee, don't hesitate to make revisions yourself, especially it's in version which solver picks up eagerly (i.e. latest ones) and e.g. breaks some other packages CI etc. (= that's our job).


To optimize the soul test class

Fixes #945

Make sure that:

[ ] You have read the contribution guidelines.
[ ] You submit test cases (unit or integration tests) that back your changes.
[ ] Your local test passed mvn clean install -Dmaven.javadoc.skip=true.

not yet.

not yet.

@HoldDie What do you mean?


New version: PowerSoftware.PowerISO version 8.4

[x] Have you signed the Contributor License Agreement?
[x] Have you checked that there aren't other open pull requests for the same manifest update/change?

[x] Does your manifest conform to the 1.4 schema?

Note: <path> is the name of the directory containing the manifest you're submitting.

Closes #94780

/azp run

/azp run


Scaling down texture cause artifact

Godot Version 3.1
I have a 4096x4096 texture, I change its size file to 512. When I use the tilable texture (for a wall) there is a line. So there seems to be an issue when scaling texture.
Steps to reproduce:
Create a 4096 tilable asset, change its size file to 512 in import. use it on a wall/floor, and see a line where the tile ends. the smaller to put the size file the bigger the line.
Minimal reproduction project:
Dont have one atm. But a screenshot to show the line, its the vertical line in the center.

Can you reproduce this if you set the texture compression mode to Lossless instead of Video RAM? To do so, select the texture in the FileSystem dock, then go to the Import dock, change the texture compression mode then click Reimport.

yes, I've tried lots of those settings, mipmaps, premute alpha, lossless, and it kept happening, only way I was able to fix it, was to change my texture to a 512 (cause then it didnt scale)

@Mugiwara00 Can you (or anyone else) still reproduce this bug in Godot 3.2.2 or any later release?
If yes, please ensure that an up-to-date Minimal Reproduction Project (MRP) is included in this report (a MRP is a zipped Godot project with the minimal elements necessary to reliably trigger the bug). You can upload ZIP files in an issue comment with a drag and drop.
We absolutely need a minimal reproduction project here.

Closing due to lack of response. Please comment if you can still reproduce this bug.
PS: If no minimal reproduction project was included in the original bug report, please upload one as well to ease troubleshooting.


[Launch] Smart Maps Portable

As I am organising ideas about how I can make use of Raspberry Pi in international development context, I will be creating a Discussion for "Smart Maps Portable".
"Smart Maps Portable" will be based on existing efforts on "UNVT Portable" led by @mapconcierge and @ShogoHirasawa.
This issue is for sharing information about the progress of the launch phase within the group. I will be closing this issue once the launch phase is done.

I created the discussion channel!
https://github.com/UNopenGIS/7/discussions/454

I am closing this issue because the discussion channel was launched.


Add ESLint

This pull request adds validatation of all .js files with ESLint 0.24, allowing for browser, node and amd environments.
There is also a eslint-pre-commit script to validate staged files before committing.
Hopefully this PR will save everyone some time:

reviewers won't have to review invalid code, or explain linting errors to contributors, it won't get committed and won't show up in pull requests
contributors will get immediate feedback via the eslint-pre-commit script and won't have to revisit pull requests just to learn about linting errors after Travis builds them

Before
$ eslint .
✖ 3359 problems (3359 errors, 0 warnings)

After
$ eslint .
✖ 45 problems (0 errors, 45 warnings)

As @gyandeeps pointed out, upcoming<EMAIL_ADDRESS>will have no default configuration, which means we will have to create one, or be stuck on 0.24 for awhile. I'll conduct some experiments with these configs, to see how well suited they are the Sinon.JS codebase
I've created an .eslintrc file with some defaults. We can always tweak it as we go along. Thankfully, ESLint has support for warnings.
@cjohansen @mantoni @gyandeeps I think this is ready for review now

Once we're happy to merge this PR, I'll squash it down to a single commit

:+1:

My suggestion:

Not use deprecated rules
Since with release 1.0.0, all the rules will be turned off by default. It would be a good idea to build a config module and then use that. example: https://www.npmjs.com/package/eslint-config-ideal
Use inline disable if we want a particular line to not throw errors. Docs: http://eslint.org/docs/user-guide/configuring#configuring-rules
example:

alert('foo'); // eslint-disable-line no-alert

Do let me know if I can be of any help.

Use inline disable if we want a particular line to not throw errors. Docs: http://eslint.org/docs/user-guide/configuring#configuring-rules example:
alert('foo'); // eslint-disable-line no-alert

I was not aware of that, thank you for the tip! I've pushed new commits that tidies this up :+1:

Since with release 1.0.0, all the rules will be turned off by default. It would be a good idea to build a config module and then use that.

I was also unaware of that. I took a look around, and https://github.com/walmartlabs/eslint-config-defaults has some appealing features for me.
Let us see how the rest of this PR turns out, before I start adding more complexity.

LGTM.
Suggestion: I would activate more stylistic rules as it helps the project to have consist styled code. I find it very helpful.

Question: Is this project going to be linted by 2 tools? Why I ask is because I see jscs also.

If ESLint can do everything that JSCS can do, then I would rather just have one tool.

Sounds good.
This PR looks good. :+1:

@cjohansen @mantoni I think this is ready

I merged and pushed this to master, but it seems GitHub is not following along. Can you verify that master is like it should?

Not entirely sure how you managed that, but the changes look to be in there.

Thanks to @gyandeeps for in-depth review of this!

np :+1:


Add Transaction Log

A feature that was asked for a lot was a transaction log. I don't think it's feasible to use NSUserDefaults for keeping track of all transactions, so this would also require a switch of storage technology.

Are you still accepting contributions for this app? if so i would like to take a look.

I haven’t touched this project for a long time but I believe there’s a PR open for this right now. I don’t think I ever got around to reviewing it properly though :(

Ok, Im trying to figure out how to make contributions to projects on here that have issues. Do people usually just download the app, use it, and comment here what they think is wrong with it or suggest a feature they think should be added?

The specific workflow depends on the project you would like to contribute to. What you described could happen, if somebody finds a bug or requests a feature, they would open an issue and somebody (maybe even the person that opened it) could work on the issue once it’s been discussed 😊
This project does not serve as a great example though since the maintainer (me) did not do a good job at actively maintaining the project (since this was more of a weekend-thing for me).

Ok, its all good. Ill mess with it anyway looking at the issues that you opened. Cool?


SatArrayInit Eldarica time-out

For SatArrayInit in the backlog / FailingTests, with the new array model (options "-array-inv" and "-array-exact 0"), Eldarica times out.
I looked for problems with the translation but could not find anything, then ran with Z3 which gives a result quickly. So I guess it's a solver problem. Could you please have a look at this Philipp?

@rodykersten can this be merged with #129?

@martinschaef no, this is unrelated


Enhancement: GitHub Enterprise Compatibility for PR-Agent

GitHub Enterprise is a self-hosting server with an API endpoint that differs from the default "api.github.com." As a result, PR-Agent currently lacks compatibility with GitHub Enterprise instances. To address this issue, a workaround on my local, involving a code change that add the base_url parameter when initializing the GitHub client.
The specific code change is in this link:
return Github(base_url='https://my.foo.github/api/v3', auth=Auth.Token(token))

However, to enhance the usability, it would be better to allow the base_url to be configured via the settings.configuration.toml file, allowing users to specify their GitHub Enterprise API endpoint.
Thanks.

@liming922
can you open a PR implementing this ?

Fixed here https://github.com/Codium-ai/pr-agent/pull/467 @liming922


Fix : jpgd.cpp Added Additional check to half_image_x_size and half_image_y_size so that it's values wont be Negative[-1].

as u can see this was the image in MRP which was successfully imported and u can also see the size of the image is 1 x 1.
also tested with other 1x1 jpg images working just fine. I made changes only on H2V2ConvertFiltered()  Function Since the problem was see there only. But i will try to Replicate the problem with different sampling and see whether any more changes are needed.   FIX : #42363

Just Found out that i need to check value of https://github.com/godotengine/godot/blob/30b6db99a99a94c64d906c1b828ff44f79a1bc75/thirdparty/jpeg-compressor/jpgd.cpp#L1672 in Function   H1V2ConvertFiltered() and aslo value of https://github.com/godotengine/godot/blob/30b6db99a99a94c64d906c1b828ff44f79a1bc75/thirdparty/jpeg-compressor/jpgd.cpp#L1765 H2V1ConvertFiltered() ,  to be non - negative to not crash.

After the last Commit we can fully use 1 x 1 pixel jpg images of any sub-sampling without Engine crash.
EVIDENCE 💯
here are some jpg images of different sub-sampling imported into Engine.
H1V1 : 👍

H1V2 : 👍

H2V1 : 👍

H2V2 : 👍

TESTED - PROJECT :
gdtest.zip

The fix looks fine, but there are a few changes that should be made before it's merged:

Commits should be squashed into one, see PR workflow.
The commit message for the resulting commit should be amended to be clearer about what the commit does. "Update filename" is not a good message as it doesn't say what the update does. See CONTRIBUTING.md for tips on writing good commit messages.
The four changed lines use inconsistent style and parentheses, they should be fixed to all be exactly the same. There's no reason for them to differ.

Finally, this is a thirdparty library so it would be best if we can get this patch merged upstream, so that all users of this library can benefit from the fix. This can also lead to writing a better fix with feedback from the original author of this code.
Could you make a PR to https://github.com/richgel999/jpeg-compressor with those fixes?

Superseded by #43441.


[BEAM-6006] Test TIME +/- INTERVAL

Test TIME +/- INTERVAL.

Follow this checklist to help us incorporate your contribution quickly and easily:

[x] Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
[ ] If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.


Path should allow all unicode characters

A path currently can only contain regex(/^[A-Za-z\-\/\_\{\}]+$/) but a URl can contain any valid unicode character including emoji. Practically speaking this prevents the use of User or Product IDs in a resource name which would be common for a RESTful API.
https://github.com/TestArmada/drydock/blob/0b5923816a4951a7db6de1a3f6bf383819449cfe/lib/schemas.js#L27
Happy to PR this if you are interested.

Thanks @baer - PR on this very welcome.


Update requirements.txt

missing dependency sqlparse==0.2.3 is added.

Hi! sqlparse is a dependency of django-debug-toolbar, which is already in requirements.txt and gets installed with that. Can you clarify why this is needed?

Closing due to lack of response from OP. sqlparse is already installed as a dependency of debug toolbar as far as I can see.

oh sorry, missed the github notification. I realized it was missing when i run the oscar app and it is fixed when i install sqlparse. No idea why, i will check again soon.


All values reported in profile_stats are 0

My attmempt to profile a pipeline returns all zeros on Ubuntu
Linux:
5.15.0-107-generic #117-Ubuntu SMP Fri Apr 26 12:26:49 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Python:
Python 3.10.12 (main, Sep 11 2024, 15:47:36) [GCC 11.4.0] on linux

pipeline = Pipeline(
    [
        read_image,
        filter_non_background
    ],
    profile=True
)

result = pipeline.map(inputs)

Output:
------------------------------+-------------------+-----------------------+--------------+----------------+----------------
Function                      | Avg CPU Usage (%) | Max Memory Usage (MB) | Avg Time (s) | Total Time (%) | Number of Calls
----------------------------- | ----------------- | --------------------- | ------------ | -------------- | ---------------
read_image                    | 0.00              | 0.00                  | 0.00e+00     | 0.0            | 0
filter_non_background_via_luv | 0.00              | 0.00                  | 0.00e+00     | 0.0            | 0
------------------------------+-------------------+-----------------------+--------------+----------------+----------------

This is a very attractive feature and I would like to learn how to make it work.

Hi @ilykos, I am glad you are trying out pipefunc!
It is not explained in the docs (and I should!) but the profiling only


not able to import "chamfer"

after running:
%cd /content/drive/MyDrive/Research/Point-MAE/GitHub/Point-MAE/extensions/chamfer_dist
!python setup.py install --user
I get this:

but when I try to import chamfer it does not work. One possible reason could be it is not installing chamfer because it stops at the resolving dependencies. Is there something that I am missing here?

tried multiple work-around but couldn't solve it. Can you please help?

Hi,
Please try to add the path (/root/.local/lib/python3.7/site-packages) of the installed packages into $PYTHONPATH.
Reference

Hi, Thanks for your reply, I tried adding directory to the path but I am still facing the same problem.

I checked the location for the files. Files are available but the import is still not working.

Hi,
Did you use a virtual environment?

Hi, I am using google colab to run the code.

Sorry, I am not familiar with colab.
We did not meet this issue in anaconda environments or docker images.

Sorry, I am not familiar with colab.
We did not meet this issue in conda environments or docker images.

after running: %cd /content/drive/MyDrive/Research/Point-MAE/GitHub/Point-MAE/extensions/chamfer_dist !python setup.py install --user
I get this:
but when I try to import chamfer it does not work. One possible reason could be it is not installing chamfer because it stops at the resolving dependencies. Is there something that I am missing here?

tried multiple work-around but couldn't solve it. Can you please help?

Hi, did you solve this problem?
I'm using colab for this code and I have the same issue, can you help me? :(

Hi, My project direction got changed. But I was not able to solve it when I was working with this. You can try working with conda environment as suggested earlier.

Hi, My project direction got changed. But I was not able to solve it when I was working with this. You can try working with conda environment as suggested earlier.

after running: %cd /content/drive/MyDrive/Research/Point-MAE/GitHub/Point-MAE/extensions/chamfer_dist !python setup.py install --user
I get this:
but when I try to import chamfer it does not work. One possible reason could be it is not installing chamfer because it stops at the resolving dependencies. Is there something that I am missing here?

tried multiple work-around but couldn't solve it. Can you please help?

Hi, did you solve this problem? I'm using colab for this code and I have the same issue, can you help me? :(

Hi, I changed the versions of Torch and Torch Vision. After you have installed the requirements.txt please install this code in colab:
!pip install torch==1.7.0 torchvision torchaudio torchtext --extra-index-url https://download.pytorch.org/whl/cu113
Good luck

On Sunday, February 26, 2023 at 05:33:25 AM GMT+3:30, Mayank Kumar @.***> wrote:
Hi, My project direction got changed. But I was not able to solve it when I was working with this. You can try working with conda environment as suggested earlier.
after running: %cd /content/drive/MyDrive/Research/Point-MAE/GitHub/Point-MAE/extensions/chamfer_dist !python setup.py install --user
I get this:
but when I try to import chamfer it does not work. One possible reason could be it is not installing chamfer because it stops at the resolving dependencies. Is there something that I am missing here?
tried multiple work-around but couldn't solve it. Can you please help?
Hi, did you solve this problem? I'm using colab for this code and I have the same issue, can you help me? :(
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: @.***>


Getting up and running

I'm trying to get this running. Do I need to register + provide API details for each exchange, or can I stream the public information using the dummy abc123 keys?
Using the dev.exs example:
** (Mix) Could not start application tai: exited in: Tai.start(:normal, [])
    ** (EXIT) an exception was raised:
        ** (ArgumentError) can't fetch value for key `:api_secret` of application `:ex_poloniex`, can not resolve key POLONIEX_API_SECRET value via adapter Elixir.Confex.Adapters.FileFromSystemEnvironment
            (confex) lib/confex.ex:245: Confex.resolve_and_update_env/2
            (elixir) lib/enum.ex:1336: Enum."-map/2-lists^map/1-0-"/2
            (tai) lib/tai.ex:8: Tai.start/2
            (kernel) application_master.erl:277: :application_master.start_it_old/4

I see the poloniex_api_key and poloniex_api_secret files in the /secrets folder, so I'm assuming that bit is all good.

Howdy @chrism2671,
Can you post a copy of your config?
The dummy keys are just to get the tests passing on CI, you should not need to do anything with them. If the venue you want to stream data allows public access without API credentials you can just set an empty map for accounts when configuring the venue.
I believe OkEx is the only venue out of the adapters I currently have that requires API credentials for streaming data.

So I'm just using the example config with no modifications:
https://github.com/fremantle-capital/tai/blob/master/config/dev.exs.example

I have a feeling the issue is coming from Confex (a module I'm not familiar with).
I tried replacing the :ex_poloniex config section with absolute paths:
config :ex_poloniex,
  api_key: {:system_file, "/home/chris/code/elixir/tai/secrets/poloniex_api_key"},
  api_secret: {:system_file, "/home/chris/code/elixir/tai/secrets/poloniex_api_secret"}

but that didn't seem to make any different.
I'm running:
MIX_ENV=dev elixir --sname tai -S mix run --no-halt

(it does work if I strip out the confex adapter and just stick in the API keys as strings.)

@chrism2671 have you set the environment variables for POLONIEX_API_KEY & POLONIEX_API_SECRET?
e.g.
export POLONIEX_API_KEY="/home/chris/code/elixir/tai/secrets/poloniex_api_key"
export POLONIEX_API_SECRET="/home/chris/code/elixir/tai/secrets/poloniex_api_secret"
MIX_ENV=dev elixir --sname tai -S mix run --no-halt

Ah, I didn't do that, I hadn't realised that's what it was looking for.
I have actually got the thing running now by just pasting in API keys directly; I don't think there's anything wrong with tai as such.
I have a couple of other questions.
I'm trying to write my first advisor. Let's say, for example, I want to do a moving average. I need to maintain that MA value somewhere. I'm trying to store it in %State{}, but without much luck. How should I store advisor state?

I think I figured it out (stick in {:ok, whatever_state_you_want}, is that right?)

@chrism2671 good to hear it's running for you now. This raises a good point though, that the configuration options are not sufficiently documented. I will add some more clarity to the README.
To keep track of data while the advisor is running you can update the store within %Tai.Advisor.State{} like you mentioned by returning the tuple {:ok, new_store} e.g.
def handle_inside_quote(_venue_id, _product_symbol, _market_quote, _changes, state) do
   count = Map.get(state.store, :count, 0)
   new_store = Map.put(state.store, :count, count + 1)
   {:ok, new_store}
end

Yep, that's exactly what I wrote.
I have another question. Is it possible to see the existing positions on an exchange? I can see Tai.VenuesAdapters.Bitmex.Positions, but it's unclear to me if there's a general interface for this.
Equally, for orders, is there a way to pull this from the exchange, rather than relying on tai's internal state?

One more question, is it possible to detect and handle executions?

I have another question. Is it possible to see the existing positions on an exchange? I can see Tai.VenuesAdapters.Bitmex.Positions, but it's unclear to me if there's a general interface for this, and an analogue for OKEx.

@chrism2671 positions are a very early WIP. There is some starter code but it doesn't do much so I have not documented it anywhere. When I need to deal with positions I currently just track that in the advisors run_store.

Equally, for orders, is there a way to pull this from the exchange, rather than relying on tai's internal state?

There is no way for tai to explicitly pull the order state. You would have to use the wrapper packages directly.

One more question, is it possible to detect and handle executions?

Yes. You can provide an order_updated_callback.

OK great. I might try to build that.
Is there an easy way to debug the contents of an advisor? When the advisor crashes, it just quits silently and changes status back to 'unstarted', without actually displaying the error. I'm working with iex -S mix.

@chrism2671 that would be super sweet!
Take a look at the video in the README for an example of the dev workflow. You can tail the log if you configure the file logger backend.

Oh that helps!
So the error is that it's looking for handle_info/2 on my advisor. I've defined it as
def handle_info(_, state), do: {:noreply, state}

but I'm not sure I should be defining it at all?
Is there a way to inspect a running advisor's state? I tried Tai.Advisors.Store.all() but that's not what I was looking for ;).

@chrism2671 you will need to define handle_info like you mentioned.
To inspect the state of the advisor you can do something like:
name = Tai.Advisor.to_name(group_id, advisor_id)
pid = Process.whereis(name)
state = :sys.get_state(pid)

So consider a case where you have limit orders on one instrument, and you want to fire a market order as soon as the limit order is filled. You'd put the market orders in the order_updated of the limit order, but in this case, state is out of scope, so you wouldn't be able to update your position. Am I missing something?

Just as an update to this, I tried doing:
order_updated_callback: &order_updated/3
  def order_updated(_previous_order, _updated_order, state) do
    IO.inspect("Order updated")
    IO.inspect state
    {:ok, state.store}
  end

But couldn't make this work. (My reasoning was looking at line 136 of advisor.ex,  callback.(old_order, updated_order, state)).

That's really cool to hear @chrism2671. Made my day 😄
Apologies for the delayed reply. I was out camping this weekend 🏕
If you want to execute within the context of the advisor to have access to state you can send the advisor process a message e.g.
advisor_name = Tai.Advisor.to_name(state.group_id, state.advisor_id)

%Tai.Trading.OrderSubmissions.BuyLimitGtc{
   # …
   order_updated_callback: &Tai.Advisor.cast_order_updated(advisor_name, &1, &2, callback, {:extra, :args})
}
|> Tai.Trading.Orders.create()

I'm glad!
That seemed to work. I sent it through as a closure and state was available!
Just wondering, on PriceLevel, should the price be Decimal?
I'm trying to think about how to implement market orders. I can fire them with ExOkex (which I'm already using for pulling balances), or send limit orders priced to lift orders already on the book. Do you have any suggestions? It seems like price is a required field for OrderStore.

Just wondering, on PriceLevel, should the price be Decimal?

I've elected to make the conversion to Decimal as lazy as possible for performance reasons. I've taken the approach to try and just pass through the information from the venues as fast as possible in the venue adapters.

I'm trying to think about how to implement market orders. I can fire them with ExOkex (which I'm already using for pulling balances), or send limit orders priced to lift orders already on the book. Do you have any suggestions? It seems like price is a required field for OrderStore.

This is another deliberate choice. I'm avoiding market orders as much as possible due to the unreliability of venues. I don't want tai to be associated with a crazy fat finger trade if possible. tai only supports limit orders at this point so market orders would have to be simulated by chasing the quotes through the order book.

I've got things up and going now, and am testing on real orders.
I've got an issue on Bitmex live that appears to be the following when some orders are cancelled.
09:00:25.994 [info] {"data":{"account_id":"main","client_id":"e97a4ad7-975b-4eee-b8de-b409636173eb","close":null,"cumulative_qty":"0","enqueued_at":"2019-09-03T09:00:25.522402Z","error_reason
":"nil","last_received_at":"2019-09-03T09:00:25.891739Z","last_venue_timestamp":"2019-09-03T09:00:25.963Z","leaves_qty":"0","price":"0.01713","product_symbol":"ethu19","product_type":"future"
,"qty":"1","side":"sell","status":"canceled","time_in_force":"gtc","type":"limit","updated_at":"2019-09-03T09:00:25.992482Z","venue_id":"bitmex","venue_order_id":"38cf2237-604b-77e8-ea06-e867
d4394aa1"},"type":"Tai.OrderUpdated"}
09:00:26.041 [info] {"data":{"action":"passive_cancel","client_id":"e97a4ad7-975b-4eee-b8de-b409636173eb","required":["open","partially_filled","filled","expired","pending_amend","amend","ame
nd_error","pending_cancel","cancel_accepted"],"was":"canceled"},"type":"Tai.OrderUpdateInvalidStatus"}
09:00:26.164 [info] {"data":{"account_id":"main","client_id":"ba048b4e-e75c-4e95-8a66-709cb27ad3c5","close":null,"cumulative_qty":"0","enqueued_at":"2019-09-03T09:00:25.529810Z","error_reason
":"nil","last_received_at":"2019-09-03T09:00:26.161613Z","last_venue_timestamp":"2019-09-03T09:00:26.137Z","leaves_qty":"0","price":"0.01713","product_symbol":"ethu19","product_type":"future"
,"qty":"1","side":"sell","status":"canceled","time_in_force":"gtc","type":"limit","updated_at":"2019-09-03T09:00:26.162688Z","venue_id":"bitmex","venue_order_id":"e17b8701-a479-acd3-46fd-6892
486b0f04"},"type":"Tai.OrderUpdated"}
09:00:26.175 [error] Task #PID<0.597.0> started from :advisor_debug_main terminating
** (FunctionClauseError) no function clause matching in Tai.Trading.Orders.Cancel.notify_updated_order/1
    (tai) lib/tai/trading/orders/cancel.ex:84: Tai.Trading.Orders.Cancel.notify_updated_order({:cancel_error, {:error, {:invalid_status, :canceled, :pending_cancel}}})
    (elixir) lib/task/supervised.ex:90: Task.Supervised.invoke_mfa/2
    (elixir) lib/task/supervised.ex:35: Task.Supervised.reply/5
    (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3
Function: #Function<0.29610064/0 in Tai.Trading.Orders.Cancel.cancel/1>
    Args: []

I'm not 100% sure where this is coming from (it seems to be transient). Any thoughts?

@chrism2671 give v0.0.35 a go.

That seemed to fix it!

@chrism2671 I've added an example ping/pong advisor which should provide you with a better reference to work from https://github.com/fremantle-capital/tai/tree/master/apps/examples/lib/examples/ping_pong

Thanks!
I'm just trying to merge in the latest commit. My def init() no longer seems to fire. I did change it def after_start(), but I'm just trying to follow the logic. Should I be using init()?
Before:
  def init(state) do
    IO.inspect "Init"
    store_update =
      state.store
      |> Map.put_new(:advisor_name, Tai.Advisor.to_name(state.group_id, state.advisor_id))
    state = Map.put(state, :store, store_update)
    {:ok, state, {:continue, :subscribe_to_products}}
  end

After:

  def after_start(state) do
    IO.inspect "Init"
    store_update =
      state.store
      |> Map.put_new(:advisor_name, Tai.Advisor.to_name(state.group_id, state.advisor_id))
    state = Map.put(state, :store, store_update)
    {:ok, state.store}
  end

Just dug a little more into this, confirmed after_state() is running, but doesn't seem to be saving the state.

@chrism2671 after_start is the new hook you'll want to use. There's an update on latest master that saves the run_store https://github.com/fremantle-capital/tai/commit/08f2f191779dcc699ca2fe438a92b527c7cd486b

Got it going, and moved everything over to handle_event().
I'm just trying to figure out; is there an easy way to refresh the balance register? It doesn't seem to track the exchanges (bitmex, okex), and I'm wondering if there was a good way to do it via REST. I can see hydrate_products_and_balances(), but not a clear way to update it.

@chrism2671 sweet. Sorry there has been a decent amount of churn recently in the core API...
There is no way to refresh the balances atm but it's definitely something I'd like to add. I just haven't required it so far. I would also like to refresh the products. But other priorities have been more important 😄

What's the right way to log fills? Should I be doing this with order_updated_callback/2 or handle_info/2?

What's the right way to log fills? Should I be doing this with order_updated_callback/2 or handle_info/2?

order_updated_callback
You'll want to handle the :partially_filled & :filled status updates. e.g.

https://github.com/fremantle-capital/tai/blob/master/apps/examples/lib/examples/ping_pong/manage_order_update.ex#L44
https://github.com/fremantle-capital/tai/blob/master/apps/examples/lib/examples/ping_pong/manage_order_update.ex#L60

Awesome, that worked really nicely.
I'm getting these back from Bitmex in my dev.log on amend. The actual behaviour on the exchange seems fine.
│16:41:34.615 [warn] {"data":{"msg":{"account":14674,"clOrdID":"gtc-pmjhWzP8RIygWQcK/ari5A==","orderID":"8│
860b9f3-1c9b-487e-433b-04d58a01e0e5","price":0.01942,"symbol":"ETHZ19","timestamp":"2019-10-28T16:41:34.59│
4Z","transactTime":"2019-10-28T16:41:34.594Z"},"received_at":"2019-10-28T16:41:34.615085Z","venue_id":"bit│
mex"},"type":"Tai.StreamMessageUnhandled"}                                                                │
│16:41:36.109 [warn] {"data":{"action":"Elixir.Tai.Trading.OrderStore.Actions.PendAmend","client_id":"c77b│
a810-f7ce-4e68-8595-1727acbf8580","last_received_at":null,"last_venue_timestamp":null,"required":["open","│
partially_filled","amend_error"],"was":"pending_amend"},"type":"Tai.OrderUpdateInvalidStatus"}

This is while amending limit orders resting on bitmex. Can I safely ignore these? The actual amends seem fine (qty=1).

I'll add it seems to get out of sync with the positions on bitmex (i.e. it misses a fill), maybe once every 12 hours (I compare the net position from the orders table with the position from REST every minute).

lnav screenshot:

It looks like you're trying to amend an order that is currently getting amended. You'll need to guard the status of the order before attempting the operation e.g. https://github.com/fremantle-capital/tai/blob/master/apps/examples/lib/examples/ping_pong/manage_quote_change.ex#L25

I'll add it seems to get out of sync with the positions on bitmex (i.e. it misses a fill), maybe once every 12 hours (I compare the net position from the orders table with the position from REST every minute).

Do you think this is related to the multiple amend operations? I've created a story and will keep an eye on it.

Sorry, only just noticed this message! I added a bunch more stuff to make sure I was trying to amend an order that wasn't in an amend-able state, and that seems to have gotten rid of the error messages.
It does still get out of sync with Bitmex, this seems to happen after 12-24 hours of operation, and usually after there's been a lot of volatility, Bitmex has been :overloaded. I run a REST sanity check every minute.
As an aside, I sidecar-ed Grafana into it, which produces pretty graphs, and would definitely recommend using it if you don't already.

I have a question. I'm trying to get the Bitmex.Stream.Connection to subscribe to the margin channel on Bitmex. I can't clearly see where auth_channels get declared, and on start_link, it seems like channels = []. Can I declare this in the config file?
I know I'll need write handlers for this message type; I just want to get the data flowing in so I can figure it out.

Heyo @chrism2671,
That's super dope! 😄 I also use Grafana to do something similar.

Sorry, only just noticed this message! I added a bunch more stuff to make sure I was trying to amend an order that wasn't in an amend-able state, and that seems to have gotten rid of the error messages.

Great to hear

It does still get out of sync with Bitmex, this seems to happen after 12-24 hours of operation, and usually after there's been a lot of volatility, Bitmex has been :overloaded. I run a REST sanity check every minute.

Thanks for investigating. How do you feel about creating a PR that does this check and logs an event? It would be really handy to be able to alert on it.

I have a question. I'm trying to get the Bitmex.Stream.Connection to subscribe to the margin channel on Bitmex. I can't clearly see where auth_channels get declared, and on start_link, it seems like channels = []. Can I declare this in the config file?
I know I'll need write handlers for this message type; I just want to get the data flowing in so I can figure it out.

Sorry overlooked this...
You do it in the venue configuration e.g.
config :tai,
  venues: %{
    bitmex: [
      enabled: true,
      adapter: Tai.VenueAdapters.Bitmex,
      products: "xbtusd",
      quote_depth: 3,
      channels: [:margin],
      accounts: %{
        main: %{
          api_key: {:system_file, "BITMEX_API_KEY"},
          api_secret: {:system_file, "BITMEX_API_SECRET"}
        }
      }
    ]

You'll also need to add it to the list of optional channels in the BitMEX connection. https://github.com/fremantle-capital/tai/blob/master/apps/tai/lib/tai/venue_adapters/bitmex/stream/connection.ex#L84

Sorry for slow response!

Thanks for investigating. How do you feel about creating a PR that does this check and logs an event? It would be really handy to be able to alert on it.

Currently I do this in the advisor- 'tis a bit hacky & looks a bit like this:
  def rest_position(state) do
    case Tai.Venues.Config.parse_adapters[@venue_id] |> Tai.Venue.positions(:main) do
      {:ok, []} ->
        0
      {:ok, positions} ->
        position =
          positions
          |> Enum.filter(fn x -> x.product_symbol == state.config.bitmex_leg end)
          |> hd
          |> Map.get(:qty)
          |> Decimal.to_integer

        log_metric(state, "bitmex_position_rest", position)

        if state.store[:initial_position] do
          websocket_position = position_from_orders(state, :filled) + state.store.initial_position
          sanity =  websocket_position == position
          if sanity == false do
            Logger.error("Position out of sync websocket: #{websocket_position} rest: #{position}")
            # shutdown(state, "Position out of sync websocket: #{websocket_position} rest: #{position}")
          end
        end
        position
      {:error, error} ->
        Logger.error "Could not get bitmex position (using cached position) #{inspect error}"
        initial_position = if state.store[:initial_position], do: state.store.initial_position, else: 0
        position_from_orders(state, :filled) + initial_position
    end
  end

It probably makes sense to have some kind of sanity checking layer, but I'm not sure the best way to engineer this in (I'm not sure I'm a good enough coder (yet!) to contribute here!)
I do see two options.

Do a 60 second position reconciliation via REST at the Adapter layer
Add a method in Advisor that gets run every x seconds that allows someone to write their own sanity check. This is quite nice as it means writing sanity checks is a part of writing an Advisor, and it's probably easy to add.

I think Advisor probably also needs a on_shutdown() thing to clean up on a fail (cancel all orders, flatten all positions); that part might be quite straightforward.

I like the idea of doing something like this at the adapter layer.
You can cancel all orders in the BitMEX adapter with a timer. Thanks to @yurikoval 😄
bitmex: [
      enabled: true,
      adapter: Tai.VenueAdapters.Bitmex,
      products: "xbtusd",
      quote_depth: 3,
      accounts: %{
        main: %{
          api_key: {:system_file, "BITMEX_API_KEY"},
          api_secret: {:system_file, "BITMEX_API_SECRET"}
        }
      },
      opts: %{
        autocancel: %{ping_interval_ms: 15_000, cancel_after_ms: 60_000}
      }
]

@chrism2671 can I close this issue as it seems like you're up and running now 😄
I've also added a built with tai section if you're ever interested in publicly releasing something you've built.

@chrism2671 can I close this issue as it seems like you're up and running now 😄
I've also added a built with tai section if you're ever interested in publicly releasing something you've built.

Yes of course! Thanks for all your help! :)


Integration of (light-)model-client with modelix.incremental

Dynamic dependency tracking is a technique often used by UI frameworks for updating the UI automatically. modelix.incremental provides some generic infrastructure for that. With the changes in this PR the modelix.text-editor can work on models managed by the (light-)model-client.
The vue.js integration can also be simplified by just registering a listener in DependencyTracking and forwarding the read/write events to vue.js.

:tada: This PR is included in version 3.13.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:


Update index.md

Update accepted grants to include Rubeus Keeper Milestone 1. The delivery was accepted.

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.


Illegal string offset 'ELEMENT'

I can't run Chromedriver acceptance tests.
Versions 1.3+
[PHPUnit\Framework\Exception] Illegal string offset 'ELEMENT'.
#1  /var/www/html/wp-content/plugins/external-content-portfolio/vendor/facebook/webdriver/lib/Remote/RemoteWebDriver.php:213
Versions < 1.2
[Error] Call to undefined method Facebook\WebDriver\Remote\RemoteWebDriver::getCapabilities()
This bug has been reported a bunch of times in various contexts.
https://github.com/facebook/php-webdriver/issues/466
https://github.com/facebook/php-webdriver/issues/468
http://phptest.club/t/phpunit-framework-exception-illegal-string-offset-element/1547
To reproduce
I can't NOT reproduce it. I've tried several versions of Chromedriver, tried Mac, tried Linux. Whatever I was doing BEFORE worked, lol. I realize that isn't very helpful. I think it's some kind of compatibility with Chrome or Chromedriver.
Starting Chromedriver
ubuntu@ip-172-31-38-142:~$ ./chromedriver --url-base
Starting ChromeDriver 77.0.3865.40 (f484704e052e0b556f8030b65b953dce96503217-refs/branch-heads/3865@{#442}) on port 9515
Only local connections are allowed.
Please protect ports used by ChromeDriver and related test frameworks to prevent access by malicious code.
google-chrome --version
Google Chrome 77.0.3865.90
./chromedriver --version
ChromeDriver 77.0.3865.40 (f484704e052e0b556f8030b65b953dce96503217-refs/branch-heads/3865@{#442})
WP-Codeception YML
class_name: AcceptanceTester
modules:
enabled:
- \Helper\Acceptance
- WPDb
- WPWebDriver
config:
WPDb:
dsn: 'mysql:host=localhost;dbname=wordpress'
user: wordpressuser
password: password
dump: tests/data/dump.sql
populate: false
cleanup: false
url: 'replaceme.com'
tablePrefix: wp
WPWebDriver:
url: 'replaceme.com'
#window_size: maximize
adminUsername: 'admin'
adminPassword: 'password'
adminPath: /wp-admin
port: 4444
#browser: phantomjs #./phantomjs --webdriver=4444
browser: chrome #java -jar selenium.jar
capabilities:
chromeOptions:
args: ["--headless", "--disable-gpu"]
binary: "/usr/bin/google-chrome" # Path to Chrome binary
#binary: "//Applications/MAMP/htdocs/wp-content/plugins/parler-wordpress-php/chromedriver"

I solved this by running
xvfb-run java -Dwebdriver.chrome.driver=/var/www/html/wp-content/plugins/WPbdd/chromedriver -jar selenium.jar
no idea why this works.


Neovim Port 80%-90% Complete

Description
This PR: has a objective to add Neovim port to Aura.
Issue: #132
Topic: #125
Assets

Checklist

[x] PR description included
[x] I've read the documents
[x] I've created or commented in an issue related to this port asking to work on it
[x] I know I shouldn't change any files in the packages folder manually
[x] I've added this port at the end of the related category list in the main README
[x] I've attached an image to the icon of the app that this port is related to
[x] I've attached a screenshot with a good zoom in fullscreen showing this port in action

@baliestri please, update the icon and banner to:

@daltonmenezes done

Thanks @baliestri  💜


When "kusion apply" with config change, expect terraform provider to update, but it triggers delete and create.

When "kusion apply" with config change, expect terraform provider to update, but it triggers delete and create.
Bug Report
When "kusion apply" with config change, expect terraform provider to update, but it triggers delete and create. This case happens in multiple resources management scene with terraform provider.
1. Minimal reproduce step (Required)
When create multiple instances with the same resource, we redefine the name that appends on kusion id using instance number . For example:
"id": "testNamespace:testProvider:testProviderType:instance1",
"id": "testNamespace:testProvider:testProviderType:instance2",
"id": "testNamespace:testProvider:testProviderType:instance3",

update the kusion config (resource parameter) which belongs to one of the current instance let's say instance2.
trigger "kusion apply", kusion says update instance2 and show correct diff,which is expected. But when I check the terraform provider log, it triggers delete and create actually which is unexpected.
find the difference in the hidden terraform folder:
before update, the existed resource in terraform.tfstate shows:
"name" = "testNamespace:testProvider:testProviderType:instance2"
after the kusion update process(terraform delete and create), the file terraform.tfstate shows:
"name" = "instance2"

2. What did you expect to see? (Required)
With config pararmter change in multiple instance mangement scene, trigger kusion apply to upadte, the terraform trigger updatge too.
3. What did you see instead (Required)
With config pararmter change in multiple instance mangement scene, trigger kusion apply to upadte, the terraform trigger delete and create.
4. What is your KusionStack components version? (Required)
releaseVersion: v0.7.4-alipay.rc.2+15852eaf

@ArronMingCN Thanks for your bug report. We have reproduced this bug, and it will be fixed recently.


when subaquatic is installed, energy bar and other bars disappear

mixins time?
subaquatic also hides the thirst bar from simple difficulty, might have to fork SD too

is already known, must try to find out why

seems like i have to use mixins to make the simple difficulty mod show up w subaquatic too. will make a separate mod that has this and other mixins soon


Empty changeset source overrides previous set value in JOSM

When a Challenge doesn't set a value for changeset source, JOSM overrides whatever was set for the previous changeset with an empty value. The default JOSM behavior is to remember the previous value, which is preferable.

Closing due to age.


Missing top margin.

The top margin is missing on a content node when there is only one node and the tabstrip is hidden.

Fixed in 54b91c8


fix: Reduce argoexec image size. Fixes #6196

Fixes #6196

See:
feat(executor): Support accessing output parameters by PNS executor running as non-root (#5564)

@vladlosev could you please comment on this? Can we run setcap in the build stage, or does it need to be in the packaging stage.

It looks like the RunAsNonRootSuite/TestRunAsNonRootWithOutputParams test is still passing, so capability bits must be preserved while copying between the images. Looks like there are no issues with this change.

https://hub.docker.com/layers/argoproj/argoexec/dev-cap/images/sha256-e25387345ebc5867e8aba23860308d9f58cf97476f18d84ff73fbd2c76df3f8e?context=explore


[WIP] Initial Queryable.insert_all/4 to support 'INSERT INTO SELECT'

refs: https://github.com/elixir-ecto/ecto/issues/3400
This is for supporting insert queries like:
INSERT INTO blog_sums ( blog_id, date, total_comments)
    SELECT blog_id, '2016-09-22', count(comment_id) as total_comments_update
    FROM blog_comments
    WHERE date = '2016-09-22'
    GROUP BY blog_id

Hi @gjaldon! I took another look and a stab at some of our discussions and I have to say that unfortunately this feature is way too complex to justify its addition to Ecto, so I will go ahead and close this. In any case, I want to thank you for your work, as we wouldn't be able to reach this conclusion if it was not for your PR. :heart:

Ok sounds good, @josevalim. I agree implementing this is more complicated than for the other queryable cases like for update_all, delete_all and all and may not be worth it considering that the given example for its use is probably better covered with a materialized view.


Regression in serde_codegen 0.7.4

Today I have upgraded one of my projects, updating all the Serde dependencies to the latest versions.  My project can be found here: https://github.com/benashford/rs-es/tree/upgrade_serde_dependencies the upgrade is currently in a branch upgrade_serde_dependencies.
In the project I have many structs which use #[derive(Deserialize), these were all working and tested using the previous version of serde_codegen, but since 0.7.4 it seems to be generating incorrect code.  But only for two of the structs.
What the two structs have in common is a type parameter, with a trait bound of Deserialize (i.e. the struct could contain any arbitrary struct which is also deserializeable).
The specific structs are: SearchHitsHitsResult and GetResult
The error is repeated for every field in each struct:
src/operations/get.rs:122:1: 122:30 error: mismatched types:
 expected `std::string::String`,
    found `T`
(expected struct `std::string::String`,
    found type parameter) [E0308]
src/operations/get.rs:122 #[derive(Debug, Deserialize)]
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/operations/get.rs:122:1: 122:30 note: in this expansion of #[derive_Deserialize] (defined in src/operations/get.rs)
src/operations/get.rs:122:1: 122:30 note: in this expansion of include!
src/operations/get.rs:122:1: 122:30 help: run `rustc --explain E0308` to see a detailed explanation

And also, for each field in both structs:
src/operations/get.rs:122:1: 1:1 error: mismatched types:
 expected `T`,
    found `operations::get::_IMPL_DESERIALIZE_FOR_GetResult::<impl serde::Deserialize for operations::get::GetResult<T>>::deserialize::__Field`
(expected type parameter,
    found enum `operations::get::_IMPL_DESERIALIZE_FOR_GetResult::<impl serde::Deserialize for operations::get::GetResult<T>>::deserialize::__Field`) [E0308]
(internal compiler error: unprintable span)
src/operations/get.rs:122:1: 1:1 note: in this expansion of #[derive_Deserialize] (defined in src/operations/get.rs)
src/operations/get.rs:122:1: 1:1 note: in this expansion of include!
src/operations/get.rs:122:1: 1:1 help: run `rustc --explain E0308` to see a detailed explanation

All of the above is with Rust: rustc 1.10.0-nightly (8da2bcac5 2016-04-28) but it can also be seen with the stable and beta Rust's too.
The output is quite long, so I won't list it all here, but for reference it can be seen on the Travis CI log of my project: https://travis-ci.org/benashford/rs-es/builds/127847754

Type inference is failing us for some reason. I have narrowed it to the following fairly-reasonable-looking code:
struct Generic<T: Deserialize> {
    v: Option<T>
}

impl<T: Deserialize> Deserialize for Generic<T>
    where Option<T>: Deserialize
{
    fn deserialize<D>(d: &mut D) -> Result<Generic<T>, D::Error>
        where D: Deserializer
    {
        struct __Visitor<D: Deserializer,
                         T: Deserialize>(PhantomData<D>, PhantomData<T>)
            where Option<T>: Deserialize;
        impl<D: Deserializer, T: Deserialize> Visitor for __Visitor<D, T>
            where Option<T>: Deserialize
        {
            type Value = Generic<T>;
            fn visit_seq<V>(&mut self,
                            mut visitor: V) -> Result<Generic<T>, V::Error>
                where V: SeqVisitor
            {
                let __field0 =
                    match try!(visitor.visit()) {
                        Some(value) => { value }
                        None => {
                            return Err(de::Error::end_of_stream());
                        }
                    };
                try!(visitor.end());
                Ok(Generic{v: __field0})
            }
        }
        const FIELDS: &'static [&'static str] = &["v"];
        d.deserialize_struct("Generic", FIELDS,
            __Visitor::<D, T>(PhantomData, PhantomData))
    }
}

which fails like this:
 expected `std::option::Option<_>`,
    found `T`
(expected enum `std::option::Option`,
    found type parameter) [E0308]
Ok(Generic{v: __field0,})
              ^~~~~~~~

I will keep digging but could use another pair of eyes. If I can't figure this out soon, I will start reverting serde_codegen commits.

So it works if visitor.visit() is changed to visitor.visit::<Option<T>>(), which is odd because that is exactly what should be inferred based on the code below it.

Narrowed further:
extern crate serde;
use serde::Deserialize;

struct Generic<T> {
    v: Option<T>
}

struct SeqVisitor;
impl SeqVisitor {
    fn visit<T>(&mut self) -> Result<Option<T>, ()> {
        panic!()
    }
}

fn visit_seq<T>() -> Generic<T> where Option<T>: Deserialize {
    Generic{v: SeqVisitor.visit().unwrap().unwrap()}
}

 expected `std::option::Option<_>`,
    found `T`
(expected enum `std::option::Option`,
    found type parameter) [E0308]
src/main.rs:17     Generic{v: SeqVisitor.visit().unwrap().unwrap()}
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

A commenter on the forum thread suggests this may be a compiler bug. I think we have a workaround which I will try implementing tonight.

Using my PR #308 I was able to build the upgrade_serde_dependencies branch of rs-es.

Cool, thanks for this, there's good detective work there!


OpenGLSVGMobject doesn't render like SVGMobject: Horizontal lines seem to have no stroke and black text is white.

Description of bug / unexpected behavior

Here is a PNG of an opengl rendering of an SVG (for an image of the the original SVG see below):

Note that I am using v0.9.0 even though the PNG window shows v0.6.0 (see issue #1875).
The a PNG of the original SVG is (click on the image to see it better since it doesn't have a background color):

As we can see, the horizontal line has stroke width and the text at the very bottom of the image is black.
When this SVG is  rendered with Cairo, I see the expected behavior.
Expected behavior

For OpenGLSVGMobject to treat SVG files the same was as SVGMobject.
How to reproduce the issue

Use any SVG with black text and a horizontal line that has stroke width to it.
Code for reproducing the problem
Paste your code here.

 I have been trying other svgs but anytime I find an issue the same issue seems to happen in the cairo renderer

@ryanmccauley211 Looks like the latest version of manim, v0.10.0 fixed the color issue with the text.  However, the text stroke width between cairo and opengl is still not correct as well as the stroke width of the horizontal line of the triangle.
Attached is the SVG (had to zip it as GH doesn't allow uploading of SVG files directly).  You will need to set the background to WHITE for cairo and opengl in order to see it properly.
        config.background_color = WHITE
        self.camera.background_color = WHITE

manimce_opengl_issue_1876.zip

Haven't got anywhere with this yet, from what I can tell passing the stroke_width is broken in cairo and opengl, it only affects the stroke in SVGMobjectbut this isn't passed to each SVGPathMobject. I hardcoded the stroke width in the shader to be really low, which affects the shapes, but the text seems so dense with vertices it doesn't help. Here are wireframes showing the difference

Then even when reducing the width to 0.0000001

So I guess the issue is more than just the stroke_width in this case

This SVG renders the way one would expect in cairo but not in opengl.  When I render this with cairo everything looks good and there is no need to change the stroke width.  The text looks good and the horizontal lines also look good.  @MrMallIronmaker  might have some ideas...?

I'm surprised the stroke-width is working at all; I seem to recall that it isn't parsed out at all in the file.

Any idea how to setup SVGMobject stroke_width? Neither passing value in constructor, nor setting up with x.stroke_width=5 does not work.

Found temporal solution.
mobject/svg/style_utils.py add the following to parse_style function:
def parse_style(svg_style: Dict[str, str]) -> Dict:
...
       if "stroke-width" in svg_style:
        manim_style["stroke_width"] = float(svg_style["stroke-width"])

Now the stroke_width is at least controllable from your .svg file. Specifying it with x.stroke_width=5 does not work - I guess the issue is to propagate the stroke_width value to all its SVGPathMobject children. I will look at it later.

This SVG renders the way one would expect in cairo but not in opengl. When I render this with cairo everything looks good and there is no need to change the stroke width. The text looks good and the horizontal lines also look good. @MrMallIronmaker might have some ideas about the stroke width...? @ryanmccauley211 yeah, hopefully someone (I don't have the cycles currently) to be able to figure this out as default renders between cairo and opengl should look the same.

Will have to check again but from what I remember it only seemed to work in cairo because the default stroke-width it was using was the same as in the file or close to it, I wasn't able to change the stroke-width in cairo by passing it if I remember correctly the same issue that @ivanpuhachov is having


Please update Changelog

Your changelog has not been updated since 0.15.0
Please provide latest changes.
It's quite important, as you're not yet using semver, I really need to know what has change between my current version and the latest one.

https://github.com/getsentry/sentry-php/commits/master

CHANGES is updated.


⚠ SceneTime has degraded performance

In 00bafd9, SceneTime ($ST) experienced degraded performance:

HTTP code: 200
Response time: 2207 ms

Resolved: SceneTime performance has improved in b2ee966 after 21 minutes.


feat/allow fees in different tokens

Before submitting this PR, please make sure:

[x] Your code builds clean without any errors or warnings
[x] You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:
In this PR we introduce the required changes for the client to accept having fees for a trade in different tokens (something that happens with Binance connector, that sometimes can charge fees in BNB and sometimes in the trade quote token). The changes include:

Remove fee_amount and fee_token from the order complete events, since that information was not being used anywhere
Change PNL calculation logic to convert fees in different tokens to the quote token using the RateOracle as price provider
Modification of the start command to always start also the RateOracle, for it to regularly update prices

Tests performed by the developer:
Added new unit tests.
Tested manually by running a PMM strategy against Binance and verifying that PNL is calculated without issues.
Test Summary:

Test PR across different connectors except for connectors with a known issue and low volume exchange.
Wait for both to both get a fill on bid and ask side and check the history performance.
Check the rate of the fees paid in base asset using rate oracle
Manually compute the total PnL after getting the converted value of the fees paid and confirming that it matches or is close enough to the displayed total PnL
Enable the use of a third-party asset for fee payment, for example, BNB, and wait for orders to get a filling.
Manually convert the fees paid that are currently not in quote value and get the total fees paid.
Manually compute for the total PnL again and confirmed that the PnL matches the output in the performance.
Check AMM connectors as well to make sure that fees and pnl are calculated properly.

Tips for QA testing:
Please check that all connectors can correctly process order complete events.
Verify PNL calculation.
This PR comes from https://github.com/CoinAlpha/hummingbot/pull/94

Proposal created: https://snapshot.org/#/hbot-prp.eth/proposal/0xe76e3dba49bfbe04109106be91508855b8cb5334b93698f644bd296a36e1d219

PR update: on-going test.
We have Mexc, Ndax, Probit and Okex left for testing


Dublicate code

All values change, everything is done similar to the example
I will leave video with a problem  I hope somebody can help :)
https://www.youtube.com/watch?v=CE-gpdlapgk

@nachaliuga
Can you upload your code for give it a near looking?

http://forum.arduino.cc/index.php?topic=476638.0
there are alot stupid stuff over there  if you need full code let me know :)

Thanks, i will have a look on it, when i am at home on sunday. At the moment i have no access to any PC. (Only Handy)

@nachaliuga
Can you post your main ino code? on forum you only show your disp functions
Regards

@nachaliuga
I need your main code. I think you have a bug in the initialisation.

https://ufile.io/8dbpy
here it is, it was a mistake that led to value change for both but now another problem is that cursor doesnt want to move on the other window I realy appreciate your help

What happens, when you change your control method to serial ?
What mean you with "other window", which window ?


[WIP] Introduce try_delete behind a type flag

Slots explicitly created with the MayPanic support item deletion by
index (try_delete), but in return have less powerful keys whose use
may result in panics (even when used in the non-try_ methods).

This is an alternative proposal for #5, equally drafty and more to support the discussion. Slots that use the alternative behavior are declared as Slot<IT, N, MayPanic> in contrast to the default Slot<IT, N> which has a KeysAreValid in the policy position.
It's a bit slimmer on the deltas, and probably a better idea altogether (especially as it does not make the key types any more complex, which might already become complex in the course of #4).
If this is taken as a route forward, it might be an option to only expose the non-try methods only for the default KeysAreValid variety of Slots, and to make the try methods accept either a usize or a key (for otherwise, access through an obtained key would be a bit awkward). The MayPanic variety indicator would need renaming in that case, but those names are straw men anyway.

I think #10 would be a nicer solution. Similar idea with a bit nicer naming and less panics.

#10 is better than this, yes, closing this in favor of it (but we might still do better).

Interesting, I haven't realized last night how close #10 was to your secound idea...


neo.rawio : API enhance proposal buffer_id and stream_id

Here a proposal for small internal enhencement for rawio layer.
For getting analogsignal using rawio at the moment we have a parse_hedaer() that discover "signal channels" and how they are grouped in distinct "signal stream".
The notion of stream was bit ambiguous. Normally it is a group of signal channel that have the same:  "sampling rate" + "size" + "dtype". In short, very often acquired by the same sub device but the notion was left to the class.
Very often a stream used to be a set of channel in the same binary buffer of hdf5 buffer.
Sometimes channels are in the same buffer but not in the same stream for instance:

In openephys binary format when we have 72 channels : 64 are ephys channels and 8 are AUX channels.
In spikeGLX the last channel (SYNC) is in the same buffer be can be removed using a flag.
We should route this channel to a secondary stream instead.

Here a proposal in add a "signal_buffer" key in the rawio.header with a unique *id and then the "signal_stream" and "signal_channel" will have stream_id
Plan :

Step 1 : add this buffer_id in all rawio classes
Step 2 : clean some rawio (Spikeglx, ...) to sperated some streams that share the same buffer.
Step 3 : rebase the PR #1513 top be build on top of this buffer_id concept instead of stream_id.

@h-mayorquin @zm711 @alejoe91

I think the discussion point would be whether buffer_id is exposed at all or whether we only allow users to choose their stream. For example if I have old code where I expect my openephys binary to return all 72 channels and then I slice it later what would I do with the new stream_id mechanism (ie would I have to rewrite my code or would I have access to say just give me the full buffer)?
I would prefer to only have the stream exposed and make it clear that although we load as a buffer the user only accesses one stream at a time.
Some other ios that have this are:
intan (for header-attached everything is one buffer, but we separate out the streams ourselves already)
neuronexus (it is basically set up like openephys where you have x channels + 6 reserved channels all in the same buffer)
plexon (where the wideband vs the filtered data is in the same buffer currently)
plexon2 (as above)
So to make a tl;dr @samuelgarcia do you imagine:
reader.parse_header()
ana_chunk = reader.get_analogsignal_chunk(buffer_id=0, stream_id='1')

or
ana_chunk = reader.get_analogsignal_chunk(stream_id='1')

and we have a dictionary that maps all possible streams to their buffers?
Or to get verbose: do we every have the same stream go across multiple buffers or can we always determine the buffer based on the stream. In my experience it is 1 or more streams/buffer but not streams spread across buffers (unless we count some formats where you have one file/channel which is technically a different buffer but I don't think I would want to treat as a different buffer).

This is great.
In my view, buffer_stream or buffer_id should be an internal implementation detail that should NOT be exposed to the users. A very important point is that with this view the API will not change, we will just create an internal separation between buffer (is the data such that it can be memmaped from a single file) and stream id (does the signal logically makes sense) . More info here: https://github.com/SpikeInterface/spikeinterface/issues/3197
So it would be the second option of @zm711 above that does not change the API.

I think Heberto and I are in complete agreement in this. Just want to make sure Sam and Alessio are also cool with buffer being completely internal, which would break returning the whole buffer if someone wanted it, though.

thanks for adding the link.
and yes this is internal to the class.
yes yes for sure the API will not be changed and so the stream_index will be the entry point for getting signals.
this can be a usefull info for low level stuff.
this buffer_id story is to make the readers class more generic when the buffer api description will be done.
At some point a signal_stream could be (without changing the API) a slice of a signal_buffer. That would make a super generic get_analosignal_chunks for many many class.

See #1544

Or to get verbose: do we every have the same stream go across multiple buffers or can we always determine the buffer based on the stream. In my experience it is 1 or more streams/buffer but not streams spread across buffers (unless we count some formats where you have one file/channel which is technically a different buffer but I don't think I would want to treat as a different buffer).

I am reading this last important stuff which is more complicated that expected (I am descovering it only now).
What to do with files that split channels across files ? Technically it is differents buffer maybe we should describe this correctly.
But then it is not what we have in mind : "a buffer contains one or more streams". (which was a very easy picture!)
The buffer and stream concept then would be not nested but indepedant. and so the dtype of stream would not contain buffer_id. only channel would have buffer_id and stream_id
I do not like this idea but maybe this would be more correct for "channel splited format".
Maybe a call would be good at this point.

Two examples off the top of my head are:
openephys legacy:
https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/src/master/openephys/OpenEphys_SampleData_1
intan one file per channel format:
https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/src/master/intan/intan_fpc_rhs_test_240329_091637


Question about import other protos

I want to import other protos like import "common/common.proto"; but it shows:
2019/03/20 18:23:02 No go_package option defined for import common/common.proto
--lile-server_out: protoc-gen-lile-server: Plugin failed with status code 1.
once I do not use lile the protoc works fine.

Hi @xiaowu2008, sorry this took a long time! But this should be fixed here https://github.com/lileio/lile/commit/1848b3ec5a655ab4920a81dd16a276cc1276fc86


VaultOperations and Passivity

Given we cannot control the vault API, how would we add additional functionality to VaultOperations and remain passive?

How to we do it for something like redis?

Redis is one of the data stores that requires active client development. The number of clients providing a generic API (invoking a generic command) increases but Redis is still a pain.
VaultTemplate exposes callback methods (within a Session context and without). Both methods accept callback objects that are called with either VaultSession or VaultClient. VaultSession and VaultClient expose methods for HTTP PUT/POST/GET/DELETE interaction and a exchange method (inspired by RestTemplate).
Within a Vault Session
vaultOperations.doWithVault(new SessionCallback<Object>() {
			@Override
			public Object doWithVault(VaultSession session) {

				session.postForEntity()/putForEntity()/...
				return …;
			}
		})

Without a Vault Session
vaultOperations.doWithVault(new ClientCallback<Object>() {
			@Override
			public Object doWithVault(VaultClient client) {

				client.getForEntity()/client.postForEntity()/…
				return …;
			}
		});

Closing this question as we're using now RestTemplate so users are able to interact with Vault's HTTP API within a session/outside a session.


[ENH] 1965 Split up embedding functions

Description of changes

It addresses this issue: https://github.com/chroma-core/chroma/issues/1965

There are a number of things going on in this PR. I've split the changes in meaningful commits to facilitate the review.

The first commit creates the empty module and renames the old embedding_functions.py ffc5e9135e1e5cc471849a8cf4fc7607d287a4ff
Then, there's one commit per embedding function moved so the reviewer can check side by side that the contents were moved word by word.
Above was made skipping the linters to avoid noise, so after them there's a commit that lints the files which should be pretty innocuous . 6ad759829df125d17e6b26d20ebb987b70861705
However, the linting over the onnx embedding function felt quite sensible to me, so I decided to put it in a separate commit 8f08d60ebdfbe8aa7f6ef37c03a6ca1ffceb6a0e

Besides, there are a few docstrings and some tests that I'm working on as discussed here for which I will open a follow up PR to avoid noise here.
@atroyn, can you please take a look?
Test plan
How are these changes tested?
I launched a couple of times the whole test suite finding that they took a lot of time to complete (in some 1h I had only covered 35%). The second time I launched them, the computer even died, but not really sure if it was because of these tests only or because something else happening at that time.
I didn't know that there were tests for JS or rust, I will run them next and tick the checkboxes as appropriate. I've put the outcomes in this comment

[x] Tests for this feature pass locally with pytest for python,
[ ] The whole test suite pass locally.
[ ] yarn test for js,
[ ] cargo test for rust

Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?
In principle all the docstrings are as they were, and users' implementations won't be affected. However, as said before, I'm working on a follow up PR that will add a few more tests and edit docstrings where appropriate.

I've been running some tests with some disappointing outcomes :confused:

My local version of node is complaining with yarn.
cargo seems not fully compiling on my ubuntu:

Error: Custom { kind: Other, error: "protoc failed: chromadb/proto/chroma.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.\n" }

Finally, pytest managed to run in some 2.5h but it threw several failures:

FAILED chromadb/test/property/test_collections_with_database_tenant_overwrite.py::test_collections_with_tenant_database_overwrite - exceptiongroup.BaseExceptionGroup: Hypothesis found 2 distinct failures. (2 sub-exceptions)
FAILED chromadb/test/property/test_collections_with_database_tenant_overwrite.py::test_repeat_failure - chromadb.errors.InvalidUUIDError: Could not parse A00 as a UUID
FAILED chromadb/test/stress/test_many_collections.py::test_many_collections[sqlite_persistent] - OSError: [Errno 24] Too many open files: '/tmp/tmp_g4jh1g1/07f976f6-1d15-47b6-ab12-e1e7ad2def5d/index_metadata.pickle'
FAILED chromadb/test/test_api.py::test_persist_index_loading[local_persist_api] - sqlite3.OperationalError: unable to open database file
FAILED chromadb/test/test_api.py::test_persist_index_loading_embedding_function[local_persist_api] - ValueError: Could not connect to tenant default_tenant. Are you sure it exists?
FAILED chromadb/test/test_api.py::test_persist_index_get_or_create_embedding_function[local_persist_api] - ValueError: Could not connect to tenant default_tenant. Are you sure it exists?
FAILED chromadb/test/test_api.py::test_persist[local_persist_api] - ValueError: Could not connect to tenant default_tenant. Are you sure it exists?
FAILED chromadb/test/test_api.py::test_persist_index_loading_params[fastapi] - ValueError: Could not connect to tenant default_tenant. Are you sure it exists?
FAILED chromadb/test/test_api.py::test_persist_index_loading_params[fastapi_persistent] - ValueError: Could not connect to tenant default_tenant. Are you sure it exists?
FAILED chromadb/test/test_api.py::test_persist_index_loading_params[sqlite] - ValueError: Could not connect to tenant default_tenant. Are you sure it exists?
FAILED chromadb/test/test_api.py::test_persist_index_loading_params[sqlite_persistent] - ValueError: Could not connect to tenant default_tenant. Are you sure it exists?

For the first two I prefer not dealing with those things for the time being, unless someone tells me so.
Regarding the second, I will take a look this week but it does not seem particulary related to my changes although after those failures there could be some assertion pointing to them :thinking:

I think this is on the right path. There is a way to go, however.
1. Move the embedding_function dir at the top level, and maybe for conciseness and alignment with the rest of the ecosystem, call it `embeddings`.

2. Making such a significant change without the requisite testing is inappropriate. It is very difficult to compare each EF to its previous implementation in this way. I also see that you made some decisions to change things around, which makes reasoning about the changes in this PR even harder.

3. Stacking these changes might have made it a little easier to review.

Hi @tazarov, thanks for your feedback, greatly appreciated 🙂
For clarity, I'm going to put my thoughts here instead of replying the other comment you left, hope it makes sense.
First of all, I'm deeply sorry that you found the PR hard to make sense as I made every effort to make it as clear as possible 😞 splitting each EF into its own commit, so in my mind, the reviewer could put side by side two github windows with, say, this commit and easily check that the functions were moved word by word. I appreciate that if you use graphite —or gitkraken like me— you may not even go to github, sorry about that. I was advised that I could use this stacking process, but I dismissed the idea as this PR might not be the best place to test a new tool and stuck to a process I know. I could have open, though, different PRs manually as this is something that I've done in the past, but for this PR I find that a bit overkilling if you don't have a tool like graphite that, appears to me, does the heavy lifting for you.
I'm a bit confused as to why do you think the __init__.py will break existing implementations. In my experience this is what you do when you have to move a file to a module but the alternative you propose will also work. You can check that this approach works as this test file wasn't touched by this PR and it still holds. I'm happy with your approach but maybe you guys want to come to grips as to how do you want the code look as here we agreed the __init__.py approach.
Regarding the tests I totally agree with you, I found this whole thing very poorly tested, e.g. this OpenAIEmbeddingFunction does not have a single test, that's why I was planning to open a separate PR with all the missing tests even when, arguably, they are out of the scope of the issue itself. Still, I personally checked that all the tests in chromadb/test/ef/ are passing. The only ones failing locally for me are these but I didn't bother very much with passing all the test suite locally as I saw that you have a bunch of Github actions doing the full chore for you and for more platforms than my specific Ubuntu. Now I realise that the workflows need an a approval of a maintainer but in principle they should tell us what the failures are. Regarding the rust and js ones, tbh, is not in my short term plans to learn those things, that's why I prefer not to invest time on them if possible as they are ad hocs for me, hope this makes sense.
Sorry for all this lengthy reply :sweat_smile:, let me know what you think. Meanwhile, I will address these two bits:

Make the python tests pass locally (comment)
Get working get_builtins() in an elegant way (comment)

@tazarov I'll take over review of this one, @nablabits and I have been working on it on https://github.com/chroma-core/chroma/issues/1965 and I've agreed to his approach (as you can see on the discussion on the issue)
@nablabits I'll allocate time to review this in depth in the next day or two, thanks for the changes here!

Hi @atroyn  thanks for your input!
Before you jump onto a deep review, let me add something: I certainly see a lot of value on what @tazarov says about the tests, would we be better off writing them upfront at least to ensure that some broad assumptions hold? I'm meaning something like this where we can ensure that:

One can call a given embedding function
It does return a list of floats

This is how this whole thing looks in my mind:

We create a PR with tests like that of the example out of current main (now they are branching off this branch)
Potentially we can address the linter issues in that PR.
We merge them onto main once everything looks good.
We merge main onto this branch and pull out our best prays in order to have them working

Does this seem reasonable in your end?

@nablabits How would we test the EFs which require API keys? And what would this test beyond the implementation of the EmbeddingFunction protocol?

@atroyn Yeah, totally agree, ideally the contributors should have provided the tests or provide them in the future, by having their dependencies locally and, for example, recording a cassette with pytest-vcr with the relevant call to the API. Then all that can be put behind a skip like this folk so local implementations won't require the external lib although having them on the workflows may well be desirable.
Yet, at the very least, we might want to have a simple test that ensures that the given embedding function does exist, and its __call__ method can be called with some specific set of args. Then, everything else can be mocked as you can see in the test I wrote. This is what I meant by broad assumptions.
But not pressing too hard, if you are happy with moving all these functions, then I'm happy :smiley:

I think we can just let it rip.

@nablabits do you still plan to wrap this up? Is it currently ready for review?

@nablabits do you still plan to wrap this up? Is it currently ready for review?

Hi @atroyn I actually had a task for myself today to checkin with this. Sure, the work is ready to be reviewed, it appeared to me that the work on the tests was not really needed by this comment.
I've just ffd the branch to resolve the conflicts as this folk had linted the old embedding_functions.py bringing it back to the history.

Great, thank you! Let me review changes from here and we can get this landed.

In the meantime if you have the stomach for it, this might be a great follow-up https://github.com/chroma-core/chroma/issues/2128

In the meantime if you have the stomach for it, this might be a great follow-up #2128

Well, let's see what @BalasubramanyamEvani says as he seemed interested on the issue. If he has moved on, I'm happy to handle it.

@nablabits @atroyn,
We have a large backlog of new EFs and bugfixes:

https://github.com/chroma-core/chroma/pull/1271
https://github.com/chroma-core/chroma/pull/1327
https://github.com/chroma-core/chroma/pull/1625
https://github.com/chroma-core/chroma/pull/1675
https://github.com/chroma-core/chroma/pull/2000
https://github.com/chroma-core/chroma/pull/1977
https://github.com/chroma-core/chroma/pull/2049
https://github.com/chroma-core/chroma/pull/2182
https://github.com/chroma-core/chroma/pull/1723
https://github.com/chroma-core/chroma/pull/1986
https://github.com/chroma-core/chroma/pull/1915
https://github.com/chroma-core/chroma/pull/1791
https://github.com/chroma-core/chroma/pull/1892

Note: There may be a few others that I have missied

This is a solid piece of work that I think we must not ignore, as community members have contributed time and effort to making these PRs. I think we should first resolve all EF-related PRs up to now and then proceed with this refactoring without triggering a relatively large rework for all contributors and reviewers involved.

@tazarov this PR is intended to make landing those other ones (and future ones) easier, and since they are purely additive it's a simple change which we can do ourselves to for the outstanding new EFs. Fine to land this first then take a look over those others.

Hi @atroyn, just a gently reminder that this folk is dangling 🙂. Let me know if you want me to do something else to land it.

@nablabits will be landing this week!

We are good to merge! I made a few more changes:

the top-level embedding_functions module now dynamically imports anything that's a sub-class of EmbeddingFunction from any file in the same directory, and also adds it to the builtins list. This means future additions will be automatic here and won't need to be edited by contributors.

merged in the httpx over request changes from main

Made sure ChromaLangchainEmbeddingFunction is in the builtins list; it previously wasn't because of the way we use a factory to construct them. Here we just hardcode adding them.

My changes broke on python versions 3.9, 3.10 - reverting and fixing.

We are good to merge! I made a few more changes:
* the top-level `embedding_functions` module now dynamically imports anything that's a sub-class of `EmbeddingFunction` from any file in the same directory, and also adds it to the builtins list. This means future additions will be automatic here and won't need to be edited by contributors.

* merged in the `httpx` over `request` changes from main

* Made sure `ChromaLangchainEmbeddingFunction` is in the builtins list; it previously wasn't because of the way we use a factory to construct them. Here we just hardcode adding them.

* Added tests to make sure that the right things were being imported, and all the things which were imported were the right type.

Thank you @nablabits for your hard work and patience!

@atroyn
Yay! Finally :rocket:
To be honest it has been a great learning experience for me, thank you for the opportunity and the trust, greatly appreciated!!
BTW, that dynamic importing looks really cool —despite of the issues on the IDEs—

We will find a way to make it work on IDEs, as a first cut @tazarov will take a look!


added pytest-cov

This add pytest-cov to dev.dependencies
It allows this command to be run pytest --cov=poetry_auto_export --cov-report term-missing

Latest available version of pytest-cov is 5.0.0, so there must have been some mistake in your change to the pyproject.toml. I've locked the dependencies again with the mentioned version. LMK if this is a problem.

i'm confused how that happened good catch no problem


Add pkg-config file

I would like to have a pkgconfig file, the patch is mostly taken from Debian and refactored for a more suitable upstream code.
thanks for considering

Could you believe that I came up with the same need today and I just submitted a patch for the same thing... haha

lol :)

@vicentebolea @LocutusOfBorg could the two of you please work together on authoring one final version of this or #73 ? Thanks!

Well, I rebased the branch on top of the current master, and took the significant bits from @vicentebolea :)
The last showstopper is: are users supposed to include the library with a prepending "<UnitTest++/header.h>" or just with "<header.h>"?
in the first case it is fine the way we did (and it is my favourite, to avoid clashes in filenames)
in the latter we should do something like
"Cflags: -I${includedir}/UnitTest++"
POC:
$ autoreconf -f -i
$ ./configure
config.status: creating UnitTest++.pc
$ pkg-config  UnitTest++.pc --libs --cflags
-I/usr/local/include/ -L/usr/local/lib -lUnitTest++

everything seems fine so far.
cheers,
G.

Looks fine for me :).
As you said it might be better to avoid name collisions, so I will also vote for the first option.
Grazie @LocutusOfBorg

actually your version was correctly adding -I :) it was mine broken :)

The intent is that people will include <UnitTest++/header.h> and is what the examples show. I can't make people use good include paths, but I can try. :)