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

Trajectory Endpoint #1845

Draft
wants to merge 123 commits into
base: 0.25.x
Choose a base branch
from

Conversation

JPBergsma
Copy link
Contributor

@JPBergsma JPBergsma commented Oct 31, 2023

This PR is mainly created to make it easier to find where I have made the changes for the trajectory endpoint. This code does not yet have the quality that I would think it needs to be merge ready.

The procfile has been adjusted so a demo can be run on the Matcloud server.

It looks like the dropping of the python 3.8 typing was reverted the 0.25.x branch but it is still in this branch. So I am not sure what I should do with that for now.

JPBergsma added 30 commits July 6, 2023 17:14
JPBergsma and others added 25 commits October 3, 2023 12:48
* Use Python 3.9 as the 'base' CI version for linting

* Update pre-commit hooks

* Run `pyupgrade --py39-plus` to upgrade legacy annotations

* Add `--exit-non-zero-on-fix` for ruff

Co-authored-by: Casper Welzel Andersen <[email protected]>

* Use f-string over format

Co-authored-by: Casper Welzel Andersen <[email protected]>

---------

Co-authored-by: Casper Welzel Andersen <[email protected]>
@ml-evs
Copy link
Member

ml-evs commented Nov 1, 2023

Looks great so far @JPBergsma!

It looks like the dropping of the python 3.8 typing was reverted the 0.25.x branch but it is still in this branch. So I am not sure what I should do with that for now.

There's no reason we can't have the typing stuff in 0.25.x (indeed I'm still running into lots of issues with the new version) -- if I cherry pick the commits into 0.25.x we can see how difficult it would be to rebase (I'll take copies of your branch that we can restore to).

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Just rebased this PR after cherry-picking the big type hint change back into 0.25.x, so this looks more manageable now. Let me know when you want a full review @JPBergsma, left a few comments below until then

@@ -4,6 +4,7 @@ flake8==6.1.0
invoke==2.2.0
isort==5.12.0
jsondiff==2.0.0
jsonlines>=3.1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
jsonlines>=3.1

Doesn't need to be in both places

Comment on lines +71 to +72
"numpy>=1.20",
"jsonlines>=3.1",
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe these should still be optional server deps, or special trajectories deps?

@@ -1 +1 @@
web: uvicorn optimade.server.main:app --host 0.0.0.0 --port $PORT
web: --host 0.0.0.0 --port $PORT
Copy link
Member

Choose a reason for hiding this comment

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

Why was this necessary? I think fly might get confused

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