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

Small improvements #393

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Small improvements #393

merged 2 commits into from
Nov 14, 2024

Conversation

Luthaf
Copy link
Contributor

@Luthaf Luthaf commented Nov 13, 2024

Fix #392

@bernstei
Copy link
Collaborator

Great - I thought I tried scale, but maybe not (or maybe I did it wrong)

@ceriottm ceriottm self-requested a review November 13, 2024 19:52
Copy link
Contributor

@ceriottm ceriottm left a comment

Choose a reason for hiding this comment

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

LGTM and works locally on my side

@bernstei
Copy link
Collaborator

FWIW, I have no easy way of testing this because when I try to install from the repo I get errors about missing rust, but when I edit ~/.local/.../chemiscope/sphinx/static/chemiscope.min.js, find the JS code that saves the PNG chemiscope map and add a scale:3 argument I don't actually get saved bitmaps larger than 600x600 (after restarting both the jupyter server and the notebook kernel).

@ceriottm
Copy link
Contributor

It's very strange that you get errors about rust - it might be needed for the optional dependency metatensor, but not for just pip install . checked more thoroughly and it does save at 1800px minimum size from firefox and chrome, both with the web interface and jupyter. @Luthaf can check on safari, I cannot ^_^

@Luthaf
Copy link
Contributor Author

Luthaf commented Nov 14, 2024

Rust should not even be needed for metatensor, since pip should use the pre-built wheels and not build from source.

@Luthaf Luthaf merged commit b7cba29 into lab-cosmo:main Nov 14, 2024
6 checks passed
@Luthaf Luthaf deleted the dev branch November 14, 2024 11:14
@Luthaf
Copy link
Contributor Author

Luthaf commented Nov 14, 2024

@bernstei you could also try to install the wheel built by CI for this PR by downloading https://github.com/lab-cosmo/chemiscope/actions/runs/11822592226/artifacts/2183374870, unziping and then running pip install ./<path/to>/chemiscope*.whl.

@bernstei
Copy link
Collaborator

I thought I had a rust issue earlier, but now the error is just complaining about npm. I'll try the wheel. Is there also always a wheel built for the main branch whenever a PR is merged?

tin 1751 : python3 -m pip install --user git+https://github.com/lab-cosmo/chemiscope
Collecting git+https://github.com/lab-cosmo/chemiscope
  Cloning https://github.com/lab-cosmo/chemiscope to /tmp/pip-req-build-4j40egx_
  Running command git clone --filter=blob:none --quiet https://github.com/lab-cosmo/chemiscope /tmp/pip-req-build-4j40egx_
  Resolved https://github.com/lab-cosmo/chemiscope to commit f493599581cf5ea27c0f7a8200392d972fc48ea1
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  error: subprocess-exited-with-error
  
  × Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [23 lines of output]
      /bin/sh: npm: command not found
      ==== rebuilding the JS code with npm, this might take a while ...
      Traceback (most recent call last):
        File "/home/Software/python/miniconda3/lib/python3.11/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
          main()
        File "/home/Software/python/miniconda3/lib/python3.11/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 335, in main
          json_out['return_val'] = hook(**hook_input['kwargs'])
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/home/Software/python/miniconda3/lib/python3.11/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 118, in get_requires_for_build_wheel
          return hook(config_settings)
                 ^^^^^^^^^^^^^^^^^^^^^
        File "/tmp/pip-build-env-4386fyd6/overlay/lib/python3.11/site-packages/setuptools/build_meta.py", line 334, in get_requires_for_build_wheel
          return self._get_build_requires(config_settings, requirements=[])
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/tmp/pip-build-env-4386fyd6/overlay/lib/python3.11/site-packages/setuptools/build_meta.py", line 304, in _get_build_requires
          self.run_setup()
        File "/tmp/pip-build-env-4386fyd6/overlay/lib/python3.11/site-packages/setuptools/build_meta.py", line 320, in run_setup
          exec(code, locals())
        File "<string>", line 130, in <module>
        File "<string>", line 84, in run_npm_build
        File "/home/Software/python/miniconda3/lib/python3.11/subprocess.py", line 571, in run
          raise CalledProcessError(retcode, process.args,
      subprocess.CalledProcessError: Command 'npm ci' returned non-zero exit status 127.
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.

@bernstei
Copy link
Collaborator

I can confirm that the prebuilt PR wheel works, and I get a much better resolution image. It'd be even better for that resolution to be settable, but that's not at all urgent.

@Luthaf
Copy link
Contributor Author

Luthaf commented Nov 14, 2024

An error on npm makes sense, we need it to build the JS part of the code.

Is there also always a wheel built for the main branch whenever a PR is merged?

Even before it is merged, you can find the link by looking at the "build-wheels" CI job, and then "Summary" and "Artifacts" at the bottom.

@bernstei
Copy link
Collaborator

OK, I can install now that I added npm. Is there a faster way of doing debugging than just modifying the typescript source in the repo dir and doing pip install --user .? That operation takes quite a few minutes. Do I need to learn about the javascript console in the browser?

@Luthaf
Copy link
Contributor Author

Luthaf commented Nov 14, 2024

So if you are debugging the juptyter extension, that's the main way. If you want to work with the website, npm start will start a debug version of the code (which will compile a lot faster) which you can open. It will also automatically rebuild and reload when the code changes. See also https://github.com/lab-cosmo/chemiscope/blob/main/Contributing.md for more information about the development workflow

@bernstei
Copy link
Collaborator

Thanks. I guess since the problem shows up in both, I can try to debug it in the web server context. Now I just have to figure out how to get information out of the typescript code (without knowing much about it).

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.

map png save uses wrong height
3 participants