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

Updates to support remote access from Windows #19

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

jskrist
Copy link
Member

@jskrist jskrist commented Oct 11, 2024

  • Removed default SKA path of OS dependent "/proj/sot/ska" or "C:\proj\sot\ska". User is expected to set this to "/proj/sot/ska" (or another location where the ska tdb data exists).
  • added functions/methods to get files/paths remotely if necessary

Fixes path issues for remote access of the TDB from Windows

Interface impacts

  • No changes to function/method/class call signatures
  • If cheta.remote_access.access_remotely is set to True when this package is loaded, the user will now be prompted to provide a hostname, username, and password (unless they have already been set in the cheta.remote_access package), this should not impact existing code, outside of the FOT MATLAB Tools.

Testing Windows (@jskrist)

  • I ran the existing tests for this package on Windows (with a slight modification to the tests to initialize the remote connection ahead of time), and they all ran with no issues.

Unit tests

  • Windows

I HAVE NOT tested any of these changes on linux, and would appreciate it if a reviewer could do that for me, since I don't have access to a linux machine in which I have control over the python installation.

Functional tests

  • I tested these changes from within the FOT MATLAB Tools to make sure they were able to import initial conditions from the Ska Engineering Archive (Cheta).
  • I also tested out fetching msids that required access to the TDB from the command line (outside of the FOT MATLAB Tools) and everything worked as expected.

Testing Mac (@taldcroft)

Unit tests

Setup

On kady, I had previously created ipyparallel configuration files with ipcluster start -n 1. Now:

# Local host (Mac)
scp kady:.ipython/profile_default/security/ipcontroller-client.json $CONDA_PREFIX/ska_remote_access.json

# On kady host in ska3 environment
ipcontroller --reuse &
ipengine

Local data files

(ska3) ➜  ska_tdb git:(pr/jskrist/19) pytest -v -s
========================================================= test session starts =========================================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0 -- /Users/aldcroft/miniconda3-arm/envs/ska3/bin/python3.11
cachedir: .pytest_cache
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 5 items                                                                                                                     

ska_tdb/tests/test_tdb.py::test_remote_access SKIPPED (remote access not being tested)
ska_tdb/tests/test_tdb.py::test_msids PASSED
ska_tdb/tests/test_tdb.py::test_find PASSED
ska_tdb/tests/test_tdb.py::test_tables PASSED
ska_tdb/tests/test_tdb.py::test_version PASSED

==================================================== 4 passed, 1 skipped in 0.72s =====================================================

Remote data files

(ska3) ➜  ska_tdb git:(pr/jskrist/19) env SKA=/no/ska SKA_ACCESS_REMOTELY=True pytest -s -v
========================================================= test session starts =========================================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0 -- /Users/aldcroft/miniconda3-arm/envs/ska3/bin/python3.11
cachedir: .pytest_cache
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collecting ... 

Enter hostname (or IP) of Ska server (enter to cancel):kady
Enter your login username [aldcroft]:aldcroft
Enter your password:
Establishing connection to kady...

collected 5 items                                                                                                                     

ska_tdb/tests/test_tdb.py::test_remote_access PASSED
ska_tdb/tests/test_tdb.py::test_msids PASSED
ska_tdb/tests/test_tdb.py::test_find PASSED
ska_tdb/tests/test_tdb.py::test_tables PASSED
ska_tdb/tests/test_tdb.py::test_version PASSED

========================================================= 5 passed in 21.31s ==========================================================

Functional tests

Install to local because cheta import Ska.tdb

ska3) ➜  ska_tdb git:(pr/jskrist/19) pip install --user .
Processing /Users/aldcroft/git/ska_tdb
  Preparing metadata (setup.py) ... done
Building wheels for collected packages: ska_tdb
  Building wheel for ska_tdb (setup.py) ... done
  Created wheel for ska_tdb: filename=ska_tdb-4.0.1.dev5+gd5a23de-py3-none-any.whl size=14959 sha256=aa281a68f7cdc56b39e0105b1fa3471ac4799cc51590f607f15d55692feb469f
  Stored in directory: /private/var/folders/0p/mj5zm2311gl283lksrknl7bc0000gp/T/pip-ephem-wheel-cache-7jrnma9x/wheels/d6/81/4d/953afb060005a8132868c6094396c328ce278d7a5d219a9962
Successfully built ska_tdb
Installing collected packages: ska_tdb
Successfully installed ska_tdb-4.0.1.dev5+gd5a23de
(ska3) ➜  ska_tdb git:(pr/jskrist/19) ls ~/.local/lib/python3.11/site-packages/   
Ska/                                   ska_tdb/                               ska_tdb-4.0.1.dev5+gd5a23de.dist-info/

Test in a directory where the local git repo is not in the path

(ska3) ➜  ska_tdb git:(pr/jskrist/19) cd docs 
(ska3) ➜  docs git:(pr/jskrist/19) env SKA=/no/ska SKA_ACCESS_REMOTELY=True ipython
Python 3.11.8 | packaged by conda-forge | (main, Feb 16 2024, 20:49:36) [Clang 16.0.6 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.22.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from cheta import fetch, remote_access

In [2]: dat = fetch.Msid("4OOTGSEL", "2021:001", "2021:002")
Enter hostname (or IP) of Ska server (enter to cancel):kady
Enter your login username [aldcroft]:aldcroft
Enter your password:
Establishing connection to kady...

In [3]: dat.tdb.Tsc
Out[3]: 
array([('4OOTGSEL', 1, 1, 0, 0, 'LETG'), ('4OOTGSEL', 1, 2, 1, 1, 'HETG')],
      dtype=[('MSID', '<U15'), ('CALIBRATION_SET_NUM', '<i8'), ('SEQUENCE_NUM', '<i8'), ('LOW_RAW_COUNT', '<i8'), ('HIGH_RAW_COUNT', '<i8'), ('STATE_CODE', '<U5')])

In [4]: dat.raw_vals
Out[4]: array([0, 0, 0, ..., 0, 0, 0], dtype=int8)

jskrist and others added 2 commits October 11, 2024 11:48
@taldcroft
Copy link
Member

@jskrist - Thanks for this PR! I spent some time playing with this on MacOS client with kady linux remote. Your initial commit didn't work for me so I took the opportunity to understand what is going on and implement some comments I had on your PR. This now passes tests for me locally and using a remote server on kady.

I did this by setting SKA_ACCESS_REMOTELY=True and then running pytest -s, at which point you can answer the remote startup questions. I added a new test that explicitly tests remote access in the remote configuration. Can you try this on Windows? 🤞

…Path objects on Windows.

- passed version into remote function `get_data_path` to avoid path manipulations outside of remote functions.
@jskrist
Copy link
Member Author

jskrist commented Oct 14, 2024

@taldcroft , thanks for testing this out on MacOS and adding the unit test. I found that when I tried to use this updated code I ran into an issue because the remote function get_data_path was creating an returning a PosixPath object, which is being serialized and transmitted back to the local machine. However on Windows, the pathlib package refuses to instantiate a PosixPath object, and doesn't do any auto conversions, so the code ends up hanging. I made a modification to return the path as a string so that the local and remote machines don't try and do anything clever with it. This also meant that it was simpler to revert to passing in the TDB version to the get_data_path function and do all the path manipulation there. I updated the test for this behavior as well. Take a look at this an make sure it is still working on MacOS. I think we are closing in on a viable option.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

I've tested this pretty well now on MacOS and it looks good.

@taldcroft taldcroft merged commit 864c286 into sot:master Oct 15, 2024
@taldcroft
Copy link
Member

My testing is documented in the description.

@jskrist jskrist deleted the windows_remote_access branch October 15, 2024 11:59
@jeanconn
Copy link

My understanding of what this does suggests the description still needs update (first bullet is more subtle now).

@jskrist
Copy link
Member Author

jskrist commented Oct 15, 2024

My understanding of what this does suggests the description still needs update (first bullet is more subtle now).

@jeanconn I have updated the initial bullet point to be more accurate with the final implementation. Let me know if it reads well to you.

@jeanconn
Copy link

Thanks @jskrist!

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.

3 participants