NSLS-II-BMM / profile_collection

BlueSky configuration for NSLS-II beamline 6BM, BMM
https://wiki-nsls2.bnl.gov/beamline6BM/index.php/Main_Page
Other
3 stars 7 forks source link

CI: add nsls2-collection-2021-2.2 envs to tests #17

Closed mrakitin closed 3 years ago

mrakitin commented 3 years ago

This adds the test config to enable testing against the new conda envs (see https://github.com/NSLS-II/profile-collection-ci/pull/23 for configs and the relevant links to the upstream activities to generate those envs).

Also, I fixed a number of assumptions such as:

Also, renamed the misspelled BMMTelementry -> BMMTelemetry.

bruceravel commented 3 years ago

That I consistently misspelled "telemetry" is simply embarrassing. Let's just keep that between you and me ;)

I think I am not going to accept your suggestion to change instances of os.getenv('HOME') to the IPython equivalent. The reason is that doing so will fail when running the profile under the qserver, which does not import ipython. That said, I appreciate the motivation and have a good idea about how to make an abstraction that will will work for both IPython and qserver. I'll hack something up and show you in a short while.

mrakitin commented 3 years ago

@bruceravel, feel free to push to this PR branch. I am interested in passing tests and encourage you to keep an eye on future failures and fix them. That way I won't have to propose fixes once a new conda environment is available.

mrakitin commented 3 years ago

@dmgav had good ideas in #9, see e.g. https://github.com/NSLS-II-BMM/profile_collection/pull/9/files#diff-b9a61ca2c121f9f8fee8a7fa9510ec0d4e0ecd6a16af2c5d1ea9377a0b3cc0ecR290 for an alternative to get_ipython().profile_dir.location/get_ipython().profile_dir.startup_dir.

bruceravel commented 3 years ago

Yes, I am very familiar with that suggestion. I ended up resolving the user_ns problem in a way that doesn't require the use of decorators or additional method arguments. His suggestion for identifying the run environment to enclose IPython specific functionality in a conditional loop is how I intend to resolve your suggestions.

I'll pull this branch and work from it.

dmgav commented 3 years ago

I think what @mrakitin meant is to replace startup_dir = get_ipython().profile_dir.startup_dir with startup_dir = os.path.split(os.path.split(__file__)[0])[0] which would work the same in the script located in the startup directory. But there are many ways to achieve this.

bruceravel commented 3 years ago

I like that much better. Good suggestion @dmgav!

mrakitin commented 3 years ago

We need to be careful with the directories. E.g. that would not work for startup/BMM/user_ns/bmm_end.py, as the code assumes the file is in the startup/BMM dir. Here is my example:

$ ipython --profile=test
Python 3.9.4 (default, Apr  9 2021, 09:32:38)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.23.0 -- An enhanced Interactive Python. Type '?' for help.

IPython profile: test
__file__ = '/Users/mrakitin/.ipython/profile_test/startup/BMM/user.py'
os.path.split(__file__) = ('/Users/mrakitin/.ipython/profile_test/startup/BMM', 'user.py')
os.path.split(__file__)[0] = '/Users/mrakitin/.ipython/profile_test/startup/BMM'
os.path.split(os.path.split(__file__)[0]) = ('/Users/mrakitin/.ipython/profile_test/startup', 'BMM')
os.path.split(os.path.split(__file__)[0])[0] = '/Users/mrakitin/.ipython/profile_test/startup'
__file__ = '/Users/mrakitin/.ipython/profile_test/startup/BMM/user_ns/bmm_end.py'
os.path.split(__file__) = ('/Users/mrakitin/.ipython/profile_test/startup/BMM/user_ns', 'bmm_end.py')
os.path.split(__file__)[0] = '/Users/mrakitin/.ipython/profile_test/startup/BMM/user_ns'
os.path.split(os.path.split(__file__)[0]) = ('/Users/mrakitin/.ipython/profile_test/startup/BMM', 'user_ns')
os.path.split(os.path.split(__file__)[0])[0] = '/Users/mrakitin/.ipython/profile_test/startup/BMM'

In [1]: !tree
.
├── 00-base.py
├── BMM
│   ├── __pycache__
│   │   └── user.cpython-39.pyc
│   ├── user.py
│   └── user_ns
│       ├── __pycache__
│       │   └── bmm_end.cpython-39.pyc
│       └── bmm_end.py
└── README

4 directories, 6 files

In [2]: !cat BMM/user.py
import os

print(f"{__file__ = }")
print(f"{os.path.split(__file__) = }")
print(f"{os.path.split(__file__)[0] = }")
print(f"{os.path.split(os.path.split(__file__)[0]) = }")
print(f"{os.path.split(os.path.split(__file__)[0])[0] = }")

startup_dir = os.path.split(os.path.split(__file__)[0])[0]

In [3]: !cat BMM/user_ns/bmm_end.py
import os

print(f"{__file__ = }")
print(f"{os.path.split(__file__) = }")
print(f"{os.path.split(__file__)[0] = }")
print(f"{os.path.split(os.path.split(__file__)[0]) = }")
print(f"{os.path.split(os.path.split(__file__)[0])[0] = }")

In [4]:
dmgav commented 3 years ago

os.path.split() separates the last pathname component so the number of times it needs to be applied depends on how deep the file is in the directory tree. I think the safest way to implement it is to find startup_dir once, set the respective variable in the global namespace and then use this variable everywhere else in the code. If it stops working after code refactoring (because files were moved) then there will be only one place to look for the problem.

mrakitin commented 3 years ago

@dmgav, I agree with that, and was about to suggest it, so 👍. Or, maybe a better solution can be to have a utility function that will calculate it for us per request?

A side question - why haven't you used os.path.dirname? That way the indices won't be needed...

bruceravel commented 3 years ago

Gents, I am being mindful of the concern @mrakitin raised three comments back. And, importantly, I am testing as I go. When things ... y'know ... work correctly, I'll push to this branch. :)

bruceravel commented 3 years ago

The push I just made fixes all the places that @mrakitin pointed out + several others. The basic idea is to find the value of startup_dir once, in startup/BMM/user_ns/base.py, then import that value as needed.

I'd like to merge this PR soon so I can continue what I had been working on. May I go ahead and merge once the CI checks are complete (assuming they pass...)?

bruceravel commented 3 years ago

@mrakitin : tests are passing now that we are skipping the problematic bit during Azure testing. I am satisfied with how this version of the profile behaves at the beamline. I would like to respond to the review and close this, if that's OK with you.