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.

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

Adding support for And #113

Merged
merged 16 commits into from
Jan 22, 2025
Merged

Adding support for And #113

merged 16 commits into from
Jan 22, 2025

Conversation

dstarkenburg
Copy link
Contributor

@dstarkenburg dstarkenburg commented Jan 10, 2025

Adding support for the And layer of the ONNX

PR Checklist

  • Tests are added
  • Documentation, if applicable

@dfdx
Copy link
Collaborator

dfdx commented Jan 11, 2025

It looks like Bool still doesn't work on Julia 1.6. Do you have an idea how to fix it or should we consider bumping the minimal version to 1.9?

@dstarkenburg
Copy link
Contributor Author

It looks like Bool still doesn't work on Julia 1.6. Do you have an idea how to fix it or should we consider bumping the minimal version to 1.9?

Im thinking it might have to do with the fact that ONNXRunTime 0.3.0 also doesnt have a Bool datatype which makes me think, unless we can patch an earlier version of ONNXRunTime for 1.6 (Long term support is pretty important) then we might be out of luck.

@dfdx
Copy link
Collaborator

dfdx commented Jan 15, 2025

earlier version of ONNXRunTime for 1.6 (Long term support is pretty important)

But the current LTS version is 1.10, no?
I think we can drop 1.6 in favor of 1.10 and dump minor version of the package. This way users on Julia < 1.10 will still be able to use older version of ONNX.jl.

@dstarkenburg
Copy link
Contributor Author

earlier version of ONNXRunTime for 1.6 (Long term support is pretty important)

But the current LTS version is 1.10, no? I think we can drop 1.6 in favor of 1.10 and dump minor version of the package. This way users on Julia < 1.10 will still be able to use older version of ONNX.jl.

Huh! look at that. I like it; what steps need to be done to bump minimal version.

@dfdx
Copy link
Collaborator

dfdx commented Jan 16, 2025

You need to change two lines of code in Project.toml:

  1. julia = "1.6" -> julia = 1.10 - this way the new version of the package will not be installable on Julia < 1.10.
  2. version = "0.2.7" -> version = "0.3.0" - this way all packages that fixed ONNX.jl version to 0.2.x will not get updated.

It may be sufficient to only change the first line, but I'd still update the ONNX.jl version as well to indicate that something important happened to the package.

I'd also change test matrix in the Github job from this:

        version:
          - '1.6'
          - '1.9'

to this:

        version:
          - '1.10'
          - '1'

1.10 means that tests will run on the LTS version (latest of 1.10.x) and 1 means that tests will run on the latest released version (1.x.x).

@dfdx
Copy link
Collaborator

dfdx commented Jan 18, 2025

I see the changes in Project.toml, thanks! If you could also update Test.yml as described above, we would immediately see its effect on CI/CD. Alternatively, we can merge this PR, and I will update the Github script by myself.

@dstarkenburg
Copy link
Contributor Author

Let me know if I did the new Umlaut compat correctly. I see that umlaut 0.7 is needed for 1.10 onward. But, did I not need to remove all the prior versions in the Project.toml not for the tests?

@dfdx
Copy link
Collaborator

dfdx commented Jan 22, 2025

Umlaut 0.7 fixes names of some internal Julia objects introduced in recent versions of the language, so using it on Julia 1.10+ is totally justified.

The PR looks good to me. Thanks!

@dfdx dfdx merged commit 7bbb35b into FluxML:master Jan 22, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants