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

Add basic pyproject.toml for installing dependencies #13

Merged
merged 8 commits into from
Mar 16, 2023
Merged

Conversation

raehik
Copy link
Contributor

@raehik raehik commented Feb 16, 2023

Closes #9 . pip install -e . works as expected. CI failures are expected ( see #16 ).

Changes:

  • remove some leftover IDE files
  • enumerate dependencies in pyproject.toml
  • add basic packaging template -- but not yet prepared as a library or package

@raehik raehik added the draft label Feb 16, 2023
@raehik
Copy link
Contributor Author

raehik commented Feb 16, 2023

(This is currently a draft and shouldn't be merged, but it looks like GitHub doesn't enable draft PRs in private repos.)

@raehik
Copy link
Contributor Author

raehik commented Feb 21, 2023

@jatkinson1000 installing deps with pip install -e . and running ./cmip26.py -85 85 -280 80 --CO2 0 --ntimes 10000 --factor 0 --chunk_size 50 --global_ 0, I'm getting the following error

ValueError: Bad Request: https://storage.googleapis.com/download/storage/v1/b/cmip6/o/GFDL_CM2_6%2Fcontrol%2Fsurface%2F.zmetadata?alt=media
User project specified in the request is invalid.

Is this down to missing an API key? I find it strange because I'm able to download that URL manually. (And it seems to be a GET, so either it's sending more info in the headers or my request differs to Python's in a bigger way.)

@jatkinson1000
Copy link
Member

Yep, I can pass you Arthur's key which I have been using.
You also want to drop ntimes to 5 or 10 to stop it running on too much data.

@raehik
Copy link
Contributor Author

raehik commented Mar 1, 2023

This is tentatively ready for review. Worth noting:

  • This only installs dependencies for running the cmip26.py script.
  • Doesn't properly package -- mostly for installing dependencies.

@raehik raehik changed the title add base pyproject.toml for building/packaging Enumerate dependencies in pyproject.toml Mar 1, 2023
@raehik raehik requested review from jdenholm and jatkinson1000 and removed request for jdenholm March 1, 2023 13:24
@raehik raehik changed the title Enumerate dependencies in pyproject.toml Add basic pyproject.toml for installing dependencies Mar 1, 2023
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

In general looks good, and thanks for the work on this!
I have successfully pip installed and all built OK!
Didn't really test running, but that's hard to do at this stage.

Only real changes I'd suggest before merging would be to add some more metadata.
We might need a quick chat/check from @arthurBarthe to finalise these.

Left a few comments elsewhere but these are generally optional of for discussion.

pyproject.toml Outdated Show resolved Hide resolved
[project]
name = "gz_ocean_momentum"
version = "0.0.0" # TODO
description = "TODO"
Copy link
Member

Choose a reason for hiding this comment

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

We can probably write something tentative here, and/or as Arthur to provide.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
where = ["."] # list of folders that contain the packages (["."] by default)
include = ["gz_ocean_momentum", "gz_ocean_momentum.*"] # package names should match these glob patterns (["*"] by default)
#exclude = ["gz_ocean_momentum.tests*", "examples.py"] # exclude packages matching these glob patterns (empty by default)
namespaces = false # to disable scanning PEP 420 namespaces (true by default)
Copy link
Member

Choose a reason for hiding this comment

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

Since we are imposing black we can add some of this into the file as follows:

[tool.black]
line-length = 88
target-version = ['py37']
include = '\.pyi?$'
exclude = '''
(
  /(
      \.eggs         # exclude a few common directories in the
    | \.git          # root of the project
    | \.hg
    | \.mypy_cache
    | \.tox
    | \.venv
    | _build
    | buck-out
    | build
    | dist
  )/
  | foo.py           # also separately exclude a file named foo.py in
                     # the root of the project
)
'''

This is optional though and I'm not sure I fully appreciate what it does at the moment.
Similar tools exist for linting etc.

.gitignore Show resolved Hide resolved
@@ -41,7 +41,7 @@
apply coarse graining. Stores the resulting dataset into an MLFLOW \
experiment within a specific run.'

data_location = tempfile.mkdtemp(dir='/scratch/ag7531/temp/')
data_location = tempfile.mkdtemp()
Copy link
Member

Choose a reason for hiding this comment

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

It seems that stuff gets stored here after data is downloaded.
We will almost certainly change this when tackling #8 but is it worth setting to a specified local location for now?
Given the data seems to be stored after this script is run I'd also question whether tempfile.mkdtemp() is the correct command to use given the docs seem to imply it is for temp dirs that should be closed and removed before completion.
This probably isn't the place to deal with this, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand, MLflow handles saving output:

mlflow.log_artifact(join(data_location, 'forcing'))

Which currently goes to ./mlruns/0/<hash>/artifacts/forcing. I think this is a tempdir and used correctly. Still useful to enable overriding (it was done so here), but I have a feeling tempfile can read envvars to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Used correctly or incorrectly?
I didn't like the use of a tempfile to save important data when I saw it, and really tmpfiles should be cleared before ending the script.

I think you can probably just commit this as it should be dealt with in #4 and doesn't really matter where it creates it for now?

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@raehik raehik merged commit 03c9f05 into main Mar 16, 2023
@raehik raehik deleted the pyproject-build branch July 27, 2023 14:38
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.

Add support for building with pip / pyproject.toml
2 participants