Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add bench dependencies on core and core_bench. #117

Merged
merged 7 commits into from
Jan 24, 2022
Merged

Add bench dependencies on core and core_bench. #117

merged 7 commits into from
Jan 24, 2022

Conversation

tmcgilchrist
Copy link
Contributor

No description provided.

@tmcgilchrist
Copy link
Contributor Author

There's one failure on 4.02 with compiling core_extended that's a dependency of core_bench.
This version is the last to support 4.02 which seems to have the bug janestreet/core_extended#12.
Putting a lower bound of 4.03 on the opam file would avoid this issue, not sure if anyone would be actively using 4.02 anymore?

@c-cube
Copy link
Member

c-cube commented Dec 1, 2021

benchmarks don't seem to be run as part of opam install yojson -t, so I don't think this should be a dependency. If people want to benchmark yojson locally they can install the dependencies.

@Leonidas-from-XIV
Copy link
Member

Yes, the benchmark is not part of the test, partly because that would be a rather heavy dependency and partly because they don't really test anything as there is no benchmark value so it wouldn't be possible to fail or succeed a test. It would be great if we could specify a with-bench variable in opam but custom variable handling in OPAM is somewhat arcane.

What would however be necessary is to figure out how to exclude dune from building this code on CI because it is failing the build on the master branch. I thought that moving it into its own @bench alias would do so but apparently it is still part of @install @check @runtest

(Overall it would be nice to run this code on CI to notice bitrot, but we'd need to have a way to specify independent test steps)

@tmcgilchrist
Copy link
Contributor Author

tmcgilchrist commented Dec 2, 2021 •
edited
Loading

ocaml-ci is driven off the opam files it finds in the repository to work out what to build. Something like with-bench or another way of introducing different classes of tests/benchmarks would be very useful but isn't currently available.

So one option is to introduce a second opam file yojson-bench.json, have that depend on yojson/core_bench, and change the opam build config to build the benchmarks but not run them in CI. 6144a7b

@Leonidas-from-XIV
Drop alcotest dependency
90092fc 
It is not used but trips up the linter so it is better to avoid it.
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Jan 17, 2022
Closed
@Leonidas-from-XIV
Quench label warnings in benchmark
6f0333c 
The CI complains that the label wasn't applied and it is right. While
not a big problem, it is convenient to remove all the issues while at
it.
@Leonidas-from-XIV
Only include the tests in the yojson package
6aea7a4 
Thanks to @talex5 for pointing out that the tests are part of
`yojson_bench` and thus the linter suggests adding alcotest.
@Leonidas-from-XIV
Copy link
Member

I'm wondering whether this is a changelog-less change. Given the previous binary wasn't ever installed, it is not much of a change for users going forward. But they can install the binary now (though it feels somewhat nonsensical to submit yojson_bench to opam-repository, given it only exists to run in CI)

@tmcgilchrist
Copy link
Contributor Author

Agree that submitting yojson_bench to opam would be a bad idea.

There's an unfortunate functionality gap between needing an opam file to drive CI and not wanting something as a package when submitting upstream via dune-release.

I don't have a good solution for that, perhaps the best option is to edit the opam-repository PR and remove the mention of yojson_bench. Having that as an option in dune-release would be useful.

@Leonidas-from-XIV
Copy link
Member

@tmcgilchrist Could you add a short entry in the changelog about this? I think we should merge it, for the CI to be helpful again and then we could see if we find a better solution for this later. Let's not have perfect be the enemy of good.

panglesd
panglesd approved these changes Jan 24, 2022
View reviewed changes
Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :)

@Leonidas-from-XIV Leonidas-from-XIV merged commit ddada01 into ocaml-community:master Jan 24, 2022
@tmcgilchrist tmcgilchrist deleted the tmcgilchrist/lint branch January 26, 2022 06:00
panglesd added a commit to panglesd/opam-repository that referenced this pull request Jun 2, 2022
@panglesd
[new release] yojson and yojson-bench (2.0.0)
dea293d 
CHANGES:

### Removed

- Removed dependency on easy-format and removed `pretty_format` from
  `Yojson`, `Yojson.Basic`, `Yojson.Safe` and `Yojson.Raw`. (@c-cube, ocaml-community/yojson#90)
- Removed dependency on `biniou`, simplifying the chain of dependencies. This
  changes some APIs:
  * `Bi_outbuf.t` in signatures is replaced with `Buffer.t`
  * `to_outbuf` becomes `to_buffer` and `stream_to_outbuf` becomes
    `stream_to_buffer`
  (@Leonidas-from-XIV, ocaml-community/yojson#74, and @gasche, ocaml-community/yojson#132)
- Removed `yojson-biniou` library
- Removed deprecated `json` type aliasing type `t` which has been available
  since 1.6.0 (@Leonidas-from-XIV, ocaml-community/yojson#100).
- Removed `json_max` type (@Leonidas-from-XIV, ocaml-community/yojson#103)
- Removed constraint that the "root" value being rendered (via either
  `pretty_print` or `to_string`) must be an object or array. (@cemerick, ocaml-community/yojson#121)
- Removed `validate_json` as it only made sense if the type was called `json`.
  (@Leonidas-from-XIV, ocaml-community/yojson#137)

### Add

- Add an opam package `yojson-bench` to deal with benchmarks dependency
  (@tmcgilchrist, ocaml-community/yojson#117)
- Add a benchmark to judge the respective performance of providing a buffer vs
  letting Yojson create an internal (ocaml-community/yojson#134, @Leonidas-from-XIV)
- Add an optional `suf` keyword argument was added to functions that write
  serialized JSON, thus allowing NDJSON output. Most functions default to not
  adding any suffix except for `to_file` (ocaml-community/yojson#124, @panglesd) and functions
  writing sequences of values where the default is `\n` (ocaml-community/yojson#135,
  @Leonidas-from-XIV)

### Change

- The `stream_from_*` and `stream_to_*` functions now use a `Seq.t` instead of a
  `Stream.t`, and they are renamed into `seq_from_*` and `seq_to_*` (@gasche, ocaml-community/yojson#131).

### Fix

- Avoid copying unnecessarily large amounts of strings when parsing (ocaml-community/yojson#85, ocaml-community/yojson#108,
  @Leonidas-from-XIV)
- Fix `stream_to_file` (ocaml-community/yojson#133, @tcoopman and @gasche)
@panglesd panglesd mentioned this pull request Jun 2, 2022
Merged
panglesd added a commit to panglesd/opam-repository that referenced this pull request Jun 3, 2022
@panglesd
[new release] yojson and yojson-bench (2.0.0)
b1b0fac 
CHANGES:

### Removed

- Removed dependency on easy-format and removed `pretty_format` from
  `Yojson`, `Yojson.Basic`, `Yojson.Safe` and `Yojson.Raw`. (@c-cube, ocaml-community/yojson#90)
- Removed dependency on `biniou`, simplifying the chain of dependencies. This
  changes some APIs:
  * `Bi_outbuf.t` in signatures is replaced with `Buffer.t`
  * `to_outbuf` becomes `to_buffer` and `stream_to_outbuf` becomes
    `stream_to_buffer`
  (@Leonidas-from-XIV, ocaml-community/yojson#74, and @gasche, ocaml-community/yojson#132)
- Removed `yojson-biniou` library
- Removed deprecated `json` type aliasing type `t` which has been available
  since 1.6.0 (@Leonidas-from-XIV, ocaml-community/yojson#100).
- Removed `json_max` type (@Leonidas-from-XIV, ocaml-community/yojson#103)
- Removed constraint that the "root" value being rendered (via either
  `pretty_print` or `to_string`) must be an object or array. (@cemerick, ocaml-community/yojson#121)
- Removed `validate_json` as it only made sense if the type was called `json`.
  (@Leonidas-from-XIV, ocaml-community/yojson#137)

### Add

- Add an opam package `yojson-bench` to deal with benchmarks dependency
  (@tmcgilchrist, ocaml-community/yojson#117)
- Add a benchmark to judge the respective performance of providing a buffer vs
  letting Yojson create an internal (ocaml-community/yojson#134, @Leonidas-from-XIV)
- Add an optional `suf` keyword argument was added to functions that write
  serialized JSON, thus allowing NDJSON output. Most functions default to not
  adding any suffix except for `to_file` (ocaml-community/yojson#124, @panglesd) and functions
  writing sequences of values where the default is `\n` (ocaml-community/yojson#135,
  @Leonidas-from-XIV)

### Change

- The `stream_from_*` and `stream_to_*` functions now use a `Seq.t` instead of a
  `Stream.t`, and they are renamed into `seq_from_*` and `seq_to_*` (@gasche, ocaml-community/yojson#131).

### Fix

- Avoid copying unnecessarily large amounts of strings when parsing (ocaml-community/yojson#85, ocaml-community/yojson#108,
  @Leonidas-from-XIV)
- Fix `stream_to_file` (ocaml-community/yojson#133, @tcoopman and @gasche)
Leonidas-from-XIV pushed a commit to panglesd/opam-repository that referenced this pull request Jun 9, 2022
@panglesd @Leonidas-from-XIV
[new release] yojson and yojson-bench (2.0.0)
accb6b8 
CHANGES:

### Removed

- Removed dependency on easy-format and removed `pretty_format` from
  `Yojson`, `Yojson.Basic`, `Yojson.Safe` and `Yojson.Raw`. (@c-cube, ocaml-community/yojson#90)
- Removed dependency on `biniou`, simplifying the chain of dependencies. This
  changes some APIs:
  * `Bi_outbuf.t` in signatures is replaced with `Buffer.t`
  * `to_outbuf` becomes `to_buffer` and `stream_to_outbuf` becomes
    `stream_to_buffer`
  (@Leonidas-from-XIV, ocaml-community/yojson#74, and @gasche, ocaml-community/yojson#132)
- Removed `yojson-biniou` library
- Removed deprecated `json` type aliasing type `t` which has been available
  since 1.6.0 (@Leonidas-from-XIV, ocaml-community/yojson#100).
- Removed `json_max` type (@Leonidas-from-XIV, ocaml-community/yojson#103)
- Removed constraint that the "root" value being rendered (via either
  `pretty_print` or `to_string`) must be an object or array. (@cemerick, ocaml-community/yojson#121)
- Removed `validate_json` as it only made sense if the type was called `json`.
  (@Leonidas-from-XIV, ocaml-community/yojson#137)

### Add

- Add an opam package `yojson-bench` to deal with benchmarks dependency
  (@tmcgilchrist, ocaml-community/yojson#117)
- Add a benchmark to judge the respective performance of providing a buffer vs
  letting Yojson create an internal (ocaml-community/yojson#134, @Leonidas-from-XIV)
- Add an optional `suf` keyword argument was added to functions that write
  serialized JSON, thus allowing NDJSON output. Most functions default to not
  adding any suffix except for `to_file` (ocaml-community/yojson#124, @panglesd) and functions
  writing sequences of values where the default is `\n` (ocaml-community/yojson#135,
  @Leonidas-from-XIV)

### Change

- The `stream_from_*` and `stream_to_*` functions now use a `Seq.t` instead of a
  `Stream.t`, and they are renamed into `seq_from_*` and `seq_to_*` (@gasche, ocaml-community/yojson#131).

### Fix

- Avoid copying unnecessarily large amounts of strings when parsing (ocaml-community/yojson#85, ocaml-community/yojson#108,
  @Leonidas-from-XIV)
- Fix `stream_to_file` (ocaml-community/yojson#133, @tcoopman and @gasche)
@panglesd panglesd mentioned this pull request Sep 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panglesd panglesd panglesd approved these changes

@NathanReb NathanReb Awaiting requested review from NathanReb NathanReb is a code owner

@Leonidas-from-XIV Leonidas-from-XIV Awaiting requested review from Leonidas-from-XIV Leonidas-from-XIV is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tmcgilchrist @c-cube @Leonidas-from-XIV @panglesd